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

docs: update getting-started and remove examples #477

Merged
merged 16 commits into from
Feb 23, 2022
Merged

Conversation

ntn-x2
Copy link
Member

@ntn-x2 ntn-x2 commented Feb 18, 2022

Getting started update

Provides an overall refresh of the getting started guide, by using the new syntax as in #473, as this PR is based on top of that. It also removes the docs/example.ts file which seemed like a useless repetition of the getting started.

This PR fixes https://github.com/KILTprotocol/ticket/issues/1825.

@ntn-x2 ntn-x2 added 📝 documentation chore: documentation ✋on hold status: on hold labels Feb 18, 2022
@ntn-x2 ntn-x2 self-assigned this Feb 18, 2022
Base automatically changed from aa-did-batches to develop February 22, 2022 14:16
@ntn-x2 ntn-x2 marked this pull request as ready for review February 22, 2022 15:06
@ntn-x2 ntn-x2 requested review from rflechtner and tjwelde February 22, 2022 15:06
@ntn-x2 ntn-x2 removed the ✋on hold status: on hold label Feb 22, 2022
@ntn-x2 ntn-x2 requested a review from Dudleyneedham February 22, 2022 15:25
Copy link
Contributor

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

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

Very nice! Finally it's a readable and useful document again. Just a few questions.

docs/getting-started.ts Show resolved Hide resolved
3. Install typescript with `yarn add typescript`
4. Make a new file, e.g. `touch getting-started.ts`
5. Execute the file with `npx ts-node getting-started.ts`
To run the script at any point during this guide, just run `npx ts-node getting-started.ts`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would require npx right? We could simply tell people to also add ts-node to their dependencies, in which case you can run yarn ts-node getting-started.ts

Copy link
Member Author

Choose a reason for hiding this comment

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

If you don't do this, certain operations like account generation could fail with the notice that "the WASM interface has not been initialized".
2. Set essential configurations, most importantly the endpoint of the KILT node to which you'll want to connect for actions that read or write to blockchain state.
These operations would throw an error if called before an endpoint has been set.
If this step is skipped, certain operations like account generation could fail with the error "the WASM interface has not been initialized".
Copy link
Contributor

Choose a reason for hiding this comment

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

if you want to be precise, sr25519 crypto will not work.

Copy link
Member Author

Choose a reason for hiding this comment

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

console.log(claimerLightDid.did)
```
Before being used in credentials, CTypes must be stored on the KILT blockchain.
To do so, an on-chain DID is required for the entity willing to write CType on chain.
Copy link
Contributor

Choose a reason for hiding this comment

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

article missing

Copy link
Member Author

Choose a reason for hiding this comment

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


The message can be encrypted with the keystore and keys as follows:
As sender identity and message validity are also checked during decryption, if the decryption process completes successfully, the attester can assume that the sender of the message is also the owner of the claim, as the two identities match.
Copy link
Contributor

Choose a reason for hiding this comment

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

The last part is something we are about to change, right? So maybe we shouldnt advertise it at this point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point -> cf7198f

}
```

The Attester creates the attestation based on the IRequestForAttestation object she received:
Then, after verifying the validity of the claims in the request for attestation, the attester builds an attestation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Unclear it that's a technical process or the attester's job. In any case we should indicate that there is a moment of judgement by the attester.

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines 548 to 553
const selectedCredential = await credential.createPresentation({
// Hide the `age` property, and only reveal the `name` one.
selectedAttributes: ['name'],
signer: keystore,
claimerDid: claimerLightDid,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

this does allow for a challenge AFAIK

Comment on lines 525 to 533
const requestForCredentialMessage = new Kilt.Message(
{
type: Kilt.Message.BodyType.REQUEST_CREDENTIAL,
content: {
cTypes: [
{ cTypeHash: ctype.hash, trustedAttesters: [attesterFullDid.did] },
],
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason this comment didn't make it... I was asking if we don't allow for a challenge on the request, or have some other challenge procedure for verification at this point?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@Dudleyneedham Dudleyneedham left a comment

Choose a reason for hiding this comment

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

LGTM, minor changes

If you don't do this, certain operations like account generation could fail with the notice that "the WASM interface has not been initialized".
2. Set essential configurations, most importantly the endpoint of the KILT node to which you'll want to connect for actions that read or write to blockchain state.
These operations would throw an error if called before an endpoint has been set.
If this step is skipped, certain operations like account generation using some algorithms, e.g., Sr25519, could fail with the error "the WASM interface has not been initialized".
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If this step is skipped, certain operations like account generation using some algorithms, e.g., Sr25519, could fail with the error "the WASM interface has not been initialized".
If this step is skipped, certain operations such as using some algorithms to generate accounts, e.g., Sr25519, could fail with the error "the WASM interface has not been initialized".

2. Set essential configurations, most importantly the endpoint of the KILT node to which you'll want to connect for actions that read or write to blockchain state.
These operations would throw an error if called before an endpoint has been set.
If this step is skipped, certain operations like account generation using some algorithms, e.g., Sr25519, could fail with the error "the WASM interface has not been initialized".
2. Set essential configurations, most importantly the address of the KILT node to connect to for anything that interacts with the KILT blockchain.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
2. Set essential configurations, most importantly the address of the KILT node to connect to for anything that interacts with the KILT blockchain.
2. Set essential configurations, most importantly the address of the KILT node to connect to interact with the KILT blockchain.

// Keyring is required to generate KILT accounts.
const keyring = new Kilt.Utils.Keyring({
ss58Format: 38,
type: 'ed25519',
Copy link
Member

Choose a reason for hiding this comment

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

We should have a comment here about the sr25519 requiring the cryptoWaitReady.

```

Or by setting a default address in the configuration, connecting implicitly.
// Keyring is required to generate KILT accounts.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Keyring is required to generate KILT accounts.
// Keyring is required to generate KILT accounts.
// If you are using `srr25519` to generate KILT accounts, the `cryptoWaitReady` must be called beforehand.


```typescript
const tx = await ctype.getStoreTx()
/* The attester signs the ctype creation transaction resulting from calling `ctype.store()` with its DID. */
Copy link
Member

Choose a reason for hiding this comment

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

We should indicate that if you are not using a local chain, it is best practice to check if the CType is stored on chain

Copy link
Member

@Dudleyneedham Dudleyneedham left a comment

Choose a reason for hiding this comment

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

LGTM

@ntn-x2 ntn-x2 merged commit 5a4b5c5 into develop Feb 23, 2022
@ntn-x2 ntn-x2 deleted the aa-fix-docs branch February 23, 2022 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📝 documentation chore: documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants