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

Add ExplorerUrl to replace Network explorer methods #496

Merged
merged 12 commits into from
Nov 26, 2021

Conversation

cycraig
Copy link
Contributor

@cycraig cycraig commented Nov 16, 2021

Description of change

Separates explorer URL code out of Network into its own ExplorerUrl struct. This is primarily to avoid the explorer being unset on private Tangle networks unexpectedly when retrieved from e.g. an IotaDID.

Also corrects the examples to print both the explorer URL for messages and identity-resolver URL for DIDs, including the Wasm examples.

This fixes examples like create_did which print the wrong URL for messages.

E.g.

DID Document Transaction > https://explorer.iota.org/mainnet/identity-resolver/message/<message_id>

should be

DID Document Transaction > https://explorer.iota.org/mainnet/message/<message_id>

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

Unit tests and Wasm examples pass locally.

Change checklist

  • 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

Fix explorer and resolver URLs in examples.
Add message_url, resolver_url tests.
@cycraig cycraig added the Bug Something isn't working. label Nov 16, 2021
@cycraig
Copy link
Contributor Author

cycraig commented Nov 16, 2021

Ideally we would move the explorer stuff out of Network completely, since it's really only for the examples.

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 for fixing the examples. I would also really like to move out the urls. Maybe we can have static instances, along the lines of:

lazy_static!{
    static ref EXPLORER_MAIN: ExplorerUrl = ExplorerUrl::new(Url::parse("https://explorer.iota.org/mainnet/identity-resolver").unwrap());
    static ref EXPLORER_DEV: ExplorerUrl = ExplorerUrl::new(Url::parse("https://explorer.iota.org/devnet/identity-resolver").unwrap());
}

impl ExplorerUrl {
    pub fn message_url(&self, message_id: &str) -> Result<Url> { ... }
    pub fn resolver_url(&self, did: &str) -> Result<Url> { ... }
}

Would be even nicer if these were infallible, since it's technically just formatting.

This can be done in a separate PR though, and this is good as-is.

bindings/wasm/examples/README.md Outdated Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
@cycraig
Copy link
Contributor Author

cycraig commented Nov 17, 2021

Thanks for the review and I definitely like the ExplorerUrl code. The only change I would make is to allow the creation of a custom ExplorerUrl for the private Tangle examples.

I'm going to leave that for a future PR as it's a nice-to-have improvement currently.

Edit: updated this PR to include ExplorerUrl.

@cycraig cycraig changed the title Fix explorer URLs in examples Add ExplorerUrl, fix example URLs Nov 24, 2021
@cycraig cycraig changed the title Add ExplorerUrl, fix example URLs Add ExplorerUrl Nov 24, 2021
@cycraig cycraig added the Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog label Nov 25, 2021
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 great! I really like the simplification of the Network type. Some minor comments.

identity-iota/src/tangle/explorer.rs Outdated Show resolved Hide resolved
identity-iota/src/tangle/explorer.rs Outdated Show resolved Hide resolved
identity-iota/src/tangle/explorer.rs Outdated Show resolved Hide resolved
@cycraig cycraig merged commit 6b7729e into dev Nov 26, 2021
@cycraig cycraig deleted the fix/example-resolver-url branch November 26, 2021 14:07
@cycraig cycraig added Rust Related to the core Rust code. Becomes part of the Rust changelog. Wasm Related to Wasm bindings. Becomes part of the Wasm changelog and removed Bug Something isn't working. labels Dec 15, 2021
@cycraig cycraig changed the title Add ExplorerUrl Add ExplorerUrl to replace Network explorer methods Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change A change to the API that requires a major release. Part of "Changed" section in 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.

2 participants