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 StardustDocumentMetadata, implement StardustDocument methods #951

Merged
merged 8 commits into from
Jul 21, 2022

Conversation

cycraig
Copy link
Contributor

@cycraig cycraig commented Jul 19, 2022

Description of change

Fleshes out the StardustDocument with getters, setters, and a Document implementation for parity with IotaDocument. Also adds StardustDocumentMetadata for the created, updated timestamps.

Update the Stardust create_did example to demonstrate how to increase the deposit amount automatically and destroy the Alias Output.

Added

  • Add StardustDocumentMetadata.
  • Add many StardustDocument methods.

Changed

  • Change StardustDocument to include StardustDocumentMetadata.
  • Update iota-client pin to latest commit.
  • Update iota-crypto to fix compilation.
  • Rename StardustDocument::deserialize_inner to unpack.

Removed

  • Remove StardustDocument::deserialize_from_block.
  • Remove StardustDocument::tmp_* temporary methods.

Links to any relevant issues

Task of #908.

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

Added unit tests, updated example works.

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 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 Jul 19, 2022
@cycraig cycraig added this to the v0.7 Features milestone Jul 19, 2022
@cycraig cycraig self-assigned this Jul 19, 2022
@cycraig cycraig mentioned this pull request Jul 19, 2022
12 tasks
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.

/// Additional attributes related to a [`StardustDocument`][crate::StardustDocument].
#[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)]
pub struct StardustDocumentMetadata {
// TODO: store created in the immutable metadata, if possible?
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea, I'll see about that in the client impl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice but I don't know what format the immutable metadata is meant to take for other purposes: they don't have a dedicated created timestamp as far as I know. It might be fine for Alias Outputs used only as DID Documents though.

@@ -86,7 +86,7 @@ async function revokeVC(storage?: Storage) {

// Update the RevocationBitmap service in the issuer's DID Document.
// This revokes the credential's unique index.
await issuer.revokeCredentials("my-revocation-service", 5);
await issuer.revokeIndices("my-revocation-service", 5);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about the rename. I feel like "revoke indices" is not very clear in what it does. I think I still prefer revokeCredentials or perhaps revokeCredentialIndices?

Copy link
Contributor Author

@cycraig cycraig Jul 20, 2022

Choose a reason for hiding this comment

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

It's to better match the function parameters and reserve the revoke_credentials name for a convenience function that actually takes in a &Credential to extract the status and revocationBitmapIndex automatically (perhaps on the Account). Right now it's assumed that the index and service are known to the revoker, so we don't show how tedious those steps actually are.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the argument against revokeCredentials. I think revokeCredentialIndices wouldn't be too bad and would be more specific than revokeIndices. Just an opinion, entirely optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about revokeBitmapIndices?

Copy link
Contributor

Choose a reason for hiding this comment

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

Slightly better, just not entirely clear that it refers to indices that correspond to credentials, in my opinion.
I'm mainly concerned about readability. So when you read through an example and you see account.revokeBitmapIndices("#my-revocation-service", 5);, it doesn't say credential anywhere even though that's what it's ultimately about. But it's probably usually clear from context, so might be fine.

Copy link
Contributor Author

@cycraig cycraig Jul 20, 2022

Choose a reason for hiding this comment

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

just not entirely clear that it refers to indices that correspond to credentials

But they might not.

Edit: an index could relate to a group of credentials or something else entirely.

But it's probably usually clear from context, so might be fine.

It's commented in the example code and one would have to extract the index from the credential (or store it) under normal circumstances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted to revoke_credentials for now until consensus can be reached. Should have been done in a different PR regardless.

@cycraig cycraig added Added A new feature that requires a minor release. Part of "Added" section in changelog Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog and removed Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Added A new feature that requires a minor release. Part of "Added" section in changelog labels Jul 21, 2022
@cycraig cycraig merged commit 1f7911e into dev Jul 21, 2022
@cycraig cycraig deleted the feat/stardust-document branch July 21, 2022 14:08
This pull request was closed.
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.
Projects
Development

Successfully merging this pull request may close these issues.

3 participants