-
Notifications
You must be signed in to change notification settings - Fork 87
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
Move verification functionality from DocumentVerifier
to CoreDocument
#606
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes. I'm still hesitant to approve because while the previous approach had the disadvantage of unexpected Cow
/cloning behaviour internally, removing the mutability of the options from DocumentVerifier
essentially makes it useless (since its purpose was mainly ergonomics using the builder pattern), unlike DocumentSigner
which still retains some options separate from SigantureOptions
.
If we were to proceed with the PR in this state (with VerifierOptions
as a parameter of verify()
) I would advocate we remove DocumentVerifier
completely and replace it with a Verifier
/SignatureVerifier
trait implemented on CoreDocument
, IotaDocument
, and maybe Account
.
Apologies for being difficult and I know I'm the one that requested the latest changes to your initial approach; I just want to minimise breaking API changes, since it looks like if this is merged as-is we might want/need to change it again later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the simplification of passing the options as a parameter as opposed to adding a second lifetime to DocumentVerifier
and using Cow
.
Interesting idea. How would such a trait fit with the traits we already have such as: https://github.com/iotaledger/identity.rs/blob/dev/identity-core/src/crypto/signature/traits/core.rs#L82? Also which functions/methods would/might impose the new |
I imagine the I'm in favour of just leaving it as an associated method then, any other options or opinions? Leaving everything as-is is also an option. |
What about moving the method and having it as |
A trait might be useful if we need interoperability between future DID method documents, but that assumes they will be implemented in different structs.
It looks like we're all leaning towards this option. |
There was a problem hiding this 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, thanks for all the changes. I like that WasmDocument::verify_data
borrows WasmVerifierOptions
now, it's always going to be confusing to Javscript developers when our API consumes an object and nulls its pointer.
DocumentVerifier
DocumentVerifier
to CoreDocument
DocumentVerifier
to CoreDocument
DocumentVerifier
to CoreDocument
Description of change
Removes
DocumentVerifier
and moves its verification methodsverify
,do_verify
toCoreDocument
essentially as:Additionally
IotaDocument::verify_data
now borrowsVerifierOptions
instead of consuming it. More precisely the new function signature is now:in place of
This is arguably more conceptual as it should be enough to "read" the verifier options in order to verify data and it allows for optimizations such as mutating the options in place between verifications.
Type of change
Add an
x
to the boxes that are relevant to your changes.How the change has been tested
cargo test
andnpm run build && npm run build:examples && npm run test:node && npm run test:browser
run locally.Change checklist
Add an
x
to the boxes that are relevant to your changes.