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.

Sign up for GitHub

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

Jump to bottom

feat: resolver rebased #5

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat: resolver rebased #5

wants to merge 5 commits into from

Conversation

itsyaasir
Copy link

@itsyaasir itsyaasir commented Nov 18, 2024 •
edited
Loading

Description of change

This pull request includes several significant changes aimed at updating the resolver to the latest rebased IOTA.

Changes include:

  • Updating all the dependencies
  • Refactor for the tests

Links to any relevant issues

Type of change

  • 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

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
  • I have checked that new and existing unit tests pass locally with my changes
  • I have updated the CHANGELOG.md, if my changes are significant enough

@itsyaasir itsyaasir self-assigned this Nov 19, 2024
@itsyaasir itsyaasir requested a review from UMR1352 November 19, 2024 13:06
@itsyaasir itsyaasir marked this pull request as draft November 19, 2024 13:06
@itsyaasir itsyaasir force-pushed the feat/resolver-rebased branch 2 times, most recently from da87029 to 43250ef Compare November 25, 2024 12:52
@itsyaasir itsyaasir marked this pull request as ready for review November 26, 2024 12:50
@itsyaasir itsyaasir force-pushed the feat/resolver-rebased branch from 1b4d2a5 to 658923f Compare November 26, 2024 13:03
UMR1352
UMR1352 requested changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@UMR1352 UMR1352 left a comment

Choose a reason for hiding this comment

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

I'd probably just drop the use of identity_iota::resolver::Resolver in favor of a custom structure (HashMap??) to get the right client for the right network.
If identity_iota_core::rebased::migration::identity::get_identity is exposed by identity_iota I'd probably use that to resolve since it returns a Result<Option<Identity>> allowing you to easily handle 500 error as well as 404 (without having to look into the error message).

tests/did_resolution.rs Outdated Show resolved Hide resolved
UMR1352
UMR1352 reviewed Nov 27, 2024
View reviewed changes
Cargo.lock Outdated Show resolved Hide resolved
@itsyaasir
Copy link
Author

I'd probably just drop the use of identity_iota::resolver::Resolver in favor of a custom structure (HashMap??) to get the right client for the right network. If identity_iota_core::rebased::migration::identity::get_identity is exposed by identity_iota I'd probably use that to resolve since it returns a Result<Option<Identity>> allowing you to easily handle 500 error as well as 404 (without having to look into the error message).

I have opted to use the HashMap structure; couple of things I changed was the did network tag to unknwn since the devnet was returning this unknwn but IIRC you are working on this so will change as soon as you are done.

@itsyaasir itsyaasir force-pushed the feat/resolver-rebased branch from 0e61fd4 to cffbf21 Compare December 2, 2024 18:32
@itsyaasir itsyaasir requested a review from UMR1352 December 2, 2024 18:33
@UMR1352
Copy link
Contributor

UMR1352 commented Dec 3, 2024

@itsyaasir good job! Do you mind waiting a bit before merging this? RN we are reworking a bit DID resolution on Rebased and I feel these changes should account for that as well.

@itsyaasir
refactor: update network configuration and dependencies
ea67dc7 
- Updated the `.env.sample` to include new network configurations for testnet, devnet, and custom networks.
- Modified `Cargo.toml` to bump the `identity_iota` and `identity_storage` dependencies to version `v1.6.0-alpha.1`.
- Adjusted the `Dockerfile` to set the `NETWORK` environment variable to `devnet`.
- Enhanced the `README.md` to clarify network configuration options and provide examples for single and multiple network setups.
- Refactored `lib.rs` to implement a new `Network` enum for better handling of network configurations and client initialization.
- Updated tests to reflect changes in network handling and ensure compatibility with the new configurations.
UMR1352
UMR1352 requested changes Dec 9, 2024
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
@itsyaasir itsyaasir requested a review from UMR1352 December 9, 2024 16:03
UMR1352
UMR1352 approved these changes Dec 10, 2024
View reviewed changes
Copy link
Contributor

@UMR1352 UMR1352 left a comment

Choose a reason for hiding this comment

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

Make sure the tests run succesfully

UMR1352
UMR1352 reviewed Dec 10, 2024
View reviewed changes
tests/common/mod.rs Outdated
Comment on lines 99 to 102
// WORKAROUND: Ignore the resullts of the faucet request as sometime the
// request will fail but the funds will be
// available.
let _ = request_faucet_funds(address).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmmm not sure if this is desirable... Can't we test on a localnet instead?

Copy link
Author

Choose a reason for hiding this comment

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

We can but we will have to publish the package which is not available here.

Copy link
Author

Choose a reason for hiding this comment

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

Anyone even what I did didn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UMR1352 UMR1352 UMR1352 approved these changes

Assignees

@itsyaasir itsyaasir

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@itsyaasir @UMR1352