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

Update resolution example to use the new Resolver #838

Merged
merged 5 commits into from
May 5, 2022

Conversation

olivereanderson
Copy link
Contributor

@olivereanderson olivereanderson commented May 5, 2022

Description of change

Closes #835

Links to any relevant issues

Unblocks #823

Type of change

Add an x to the boxes that are relevant to your changes.

  • Bug fix (a non-breaking change which fixes an issue)
  • Enhancement (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Fix

How the change has been tested

Describe the tests that you ran to verify your changes.
Make sure to provide instructions for the maintainer as well as any relevant configurations.

Change checklist

Add an x to the boxes that are relevant to your changes.

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@olivereanderson olivereanderson self-assigned this May 5, 2022
@olivereanderson olivereanderson added No changelog Excludes PR from becoming part of any changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. Wasm Related to Wasm bindings. Becomes part of the Wasm changelog labels May 5, 2022
@olivereanderson olivereanderson added this to the Documentation catch-up milestone May 5, 2022
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

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

Looks good to me, just nits.

Slightly related: We don't really consider the Resolver "low-level", right? Currently, all examples that don't use an account go into the low-level-api examples, but I think we should at some point rename examples/account to examples/high-level-api or similar, and move this example there, and potentially others that don't qualify as low-level.

Edit: On second thought, I think the original plan was to just have a single examples folder anyway, in which case this inconsistency would also go away.

bindings/wasm/examples/src/resolve_did.js Outdated Show resolved Hide resolved
examples/low-level-api/resolve_did.rs Outdated Show resolved Hide resolved
@abdulmth
Copy link
Contributor

abdulmth commented May 5, 2022

Looks good to me, just nits.

Slightly related: We don't really consider the Resolver "low-level", right? Currently, all examples that don't use an account go into the low-level-api examples, but I think we should at some point rename examples/account to examples/high-level-api or similar, and move this example there, and potentially others that don't qualify as low-level.

Edit: On second thought, I think the original plan was to just have a single examples folder anyway, in which case this inconsistency would also go away.

@PhilippGackstatter This is being done here, so we're gonna have basic and advanced examples.

@olivereanderson This is targeting the support branch, shouldn't it also be part of dev?

@olivereanderson
Copy link
Contributor Author

This is targeting the support branch, shouldn't it also be part of dev?

This is a good question. Right now I need it on the support branch, but it should be on dev as well. I am very much under the impression that we will merge the support branch into dev at some point? Hence if that is the case I am not sure whether it is fine to wait for that, or if I should submit an extra PR afterwards that targetting dev.

@abdulmth @PhilippGackstatter Do you have any preferences here?

@PhilippGackstatter
Copy link
Contributor

@eike-hass merged my storage wiki article #818 back into dev from the support branch in #832, he can probably help you with that as well for this PR. So if you need it on the support branch, then the current target if the way to go, imo.

examples/low-level-api/resolve_did.rs Outdated Show resolved Hide resolved
#[tokio::main]
async fn main() -> Result<()> {
// Create a signed DID Document and KeyPair (see create_did.rs).
let (document, _, _): (IotaDocument, KeyPair, Receipt) = create_did::run().await?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let (document, _, _): (IotaDocument, KeyPair, Receipt) = create_did::run().await?;
let account: Account = Account::builder()
.create_identity(IdentitySetup::default())
.await?

In the new examples, we try to make them not depend on each others, and using the account makes it easy to do that, but maybe this requires moving the example to the account folder. So maybe this suggestion is optional for this PR but needed for the new example structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @abdulmth for this insight. It makes a lot of sense to use the Account where possible, but I would indeed like to wait with making your suggested change until we have the new example structure in place. Will definitely make sure to remember this for later though 👍

async function resolveDID(clientConfig) {

// Creates a new identity (see "create_did" example)
const {key, doc, receipt} = await createIdentity(clientConfig);
Copy link
Contributor

Choose a reason for hiding this comment

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

same this here as in the rust example.

Co-authored-by: Abdulrahim Al Methiab <31316147+abdulmth@users.noreply.github.com>
@olivereanderson olivereanderson merged commit bf555b4 into support/v0.5 May 5, 2022
@olivereanderson olivereanderson deleted the chore/update-resolution-example branch May 5, 2022 14:55
eike-hass pushed a commit that referenced this pull request May 5, 2022
Updates the resolution example to use the new Resolver.
eike-hass added a commit that referenced this pull request May 6, 2022
…-example

Update resolution example to use the new Resolver (#838)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No changelog Excludes PR from becoming part of any changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. Wasm Related to Wasm bindings. Becomes part of the Wasm changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants