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

Fix dependent diff updates being rejected #605

Merged
merged 8 commits into from
Jan 24, 2022
Merged

Conversation

cycraig
Copy link
Contributor

@cycraig cycraig commented Jan 20, 2022

Description of change

Fixes the DiffChain rejecting DiffMessages that update document sections added in previous diffs. From the example given in #593: adding a service in a diff update then editing or removing it in another diff update will result in the latter update being rejected/discarded by the DiffChain during resolution.

Thank you to @PhilippGackstatter and @abdulmth for identifying and investigating the issue!

This also flattens the properties of DocumentMetadata serialization (this ensures unhandled fields are captured during deserialization), which may lead to breaking changes if any existing user-defined custom fields conflict with hardcoded field names.

WARNING: this may result in resolutions of IOTA DID Documents that contain diff updates being different after this is merged.

Links to any relevant issues

Fixes #593

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

Existing and new tests pass, including examples.

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

@cycraig cycraig added Wasm Related to Wasm bindings. Becomes part of the Wasm changelog 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. labels Jan 20, 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.

Unfortunately, I can't confirm it's working on my side. The example from #593 is printing the expected result, but that seems to be a coincidence. No matter how many diffs I publish in try_from_index_with_document,

index.get(
      this
        .current_message_id()
        .unwrap_or_else(|| current_document.message_id()),
    ).unwrap().len()

is always 1.

For instance, if I run an example that adds service-1, deletes service-1 and adds service-2, the service property is still empty when being resolved, even though I confirmed three diffs are being published.

identity-iota/src/chain/diff_chain.rs Outdated Show resolved Hide resolved
@cycraig
Copy link
Contributor Author

cycraig commented Jan 20, 2022

Thanks for catching that! Turns out there's an entirely unrelated bug to what this PR fixes and I tested insufficiently (multiple times).

Almost all DiffMessages fail validation after deserialization now because of the change in #598. It appears that updates encoded in DiffIotaDocument change after (de)serialization to JSON, which means the signature validation fails since when it's serialized a second time the JSON changes. I'll open another PR to address this...

Current unit tests failed to catch this since they instantiate DiffMessages directly rather than deserialising them from the Tangle, and we don't have asserts in the examples so I just assumed they were working.

@cycraig cycraig mentioned this pull request Jan 21, 2022
10 tasks
@cycraig
Copy link
Contributor Author

cycraig commented Jan 21, 2022

Opened #611 to fix the deserialisation bug, will re-evaluate this PR once that's merged. Merged.

@cycraig
Copy link
Contributor Author

cycraig commented Jan 21, 2022

Seems to be fixed, I manually checked the examples this time!

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, can confirm my example is also working now 👌! Thank you very much for fixing this!

@@ -311,6 +311,7 @@ impl IotaDocument {
}

/// Remove a [`Service`] identified by the given [`IotaDIDUrl`] from the document.
// TODO: return an error or bool if no service was removed?
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, then we can remove this:

ensure!(
state.document().service().query(&service_url).is_some(),
UpdateError::ServiceNotFound
);

Doesn't need to be in this PR.

@cycraig cycraig merged commit bbfba22 into dev Jan 24, 2022
@cycraig cycraig deleted the fix/resolve-diff-updates branch January 24, 2022 12:57
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.

[Bug] DiffChain resolution discards certain valid updates as spam
3 participants