Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove mistakenly merged files. Add code references, make title consi… #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lovesh
Copy link
Contributor

@lovesh lovesh commented Apr 14, 2018

…stent across themis, remove reference to a meeting notes doc

Signed-off-by: Lovesh Harchandani [email protected]

…stent across themis, remove reference to a meeting notes doc

Signed-off-by: Lovesh Harchandani <[email protected]>
@@ -1,4 +1,4 @@
# CRED_OFFER
# Credential Offer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need both long names (e.g., Credential Offer) as well as msg types (e.g., CRED_OFFER) right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently there is nothing called a CRED_OFFER in the open source codebase atleast

@@ -14,3 +14,7 @@ The message sent by the relying party to the holder describing the verifiable at
"nonce": "<a nonce acting as request id>"
}
```


## Code reference (some of below might be different from above mentioned structure, the above structure should be considered the correct representation)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why might it be different? It seems that the code would be more correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code would (and in some cases "should") be refactored.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so what should we do when we have documentation that is ahead of the code structure? I think the documentation should mention both the current version, and the future version, right?

@@ -92,5 +92,9 @@ When claims are issued and/or revoked, the revocation registry needs to be updat
}
```

## Code references to ledger objects
## Code references to ledger objects (some of below might be different from above mentioned structure, the above structure should be considered the correct representation)
1. [CLAIM_DEF](https://github.com/hyperledger/indy-sdk/blob/778a38d92234080bb77c6dd469a8ff298d9b7154/libindy/src/services/ledger/types.rs#L234)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be CRED_DEF?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so you'll make that change?

1. [CLAIM_DEF](https://github.com/hyperledger/indy-sdk/blob/778a38d92234080bb77c6dd469a8ff298d9b7154/libindy/src/services/ledger/types.rs#L234)
2. [Primary public key](https://github.com/hyperledger/indy-crypto/blob/fc078a014a6b72ede838b79696258d3ee56f87d4/libindy-crypto/src/cl/mod.rs#L161)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem like protocol level documentation. Maybe this should be in the indy-crypto repo itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, why a credential definition has primary and revocation sections go in this protocol repo and the details of the structure of each of the objects go in the respective source code repo (indy-crypto, indy-sdk, indy-node)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!

1. [CLAIM_DEF](https://github.com/hyperledger/indy-sdk/blob/778a38d92234080bb77c6dd469a8ff298d9b7154/libindy/src/services/ledger/types.rs#L234)
2. [Primary public key](https://github.com/hyperledger/indy-crypto/blob/fc078a014a6b72ede838b79696258d3ee56f87d4/libindy-crypto/src/cl/mod.rs#L161)
3. [Revocation public key](https://github.com/hyperledger/indy-crypto/blob/fc078a014a6b72ede838b79696258d3ee56f87d4/libindy-crypto/src/cl/mod.rs#L202)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These links will work forever, which is great.
Problem is, if that object evolves, then the documentation won't show that.
The reason for having the link is we can jump to the current code representation of that object.

Maybe we should move some of this lower level stuff to the source code repo and reference the documentation from this repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, if we move the sections describing the structure of Cred def, etc to respective source repos, then these can be converted to links to master (active) branch and can be updated with the code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants