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 Identity Update Builder #632

Merged
merged 3 commits into from
Apr 8, 2024
Merged

Conversation

neekolas
Copy link
Contributor

@neekolas neekolas commented Apr 7, 2024

tl;dr

  • Adds IdentityUpdateBuilder and SignatureRequest structs, used to create batched Identity Updates

Design decisions

There were a few constraints on the design that made this a tricky one to implement.

  • These are batch signatures. An IdentityUpdate may contain several actions, each requiring one or two signatures. But each of those signatures are against a signature text derived from the entire batch. So, we need to gather an outline of all the updates before we can start collecting signatures.
  • Passing signers over the FFI barrier has proven complicated in the past. Some signers are synchronous, some are asynchronous. The caller doesn't necessarily have all the signers available every time they use libxmtp, so they may have to prompt the user before they can get a signature. The most versatile solution is to have a "signature request" that goes over the FFI barrier to platform SDKs, and have the SDKs return the serialized bytes.

Usage

        let account_address = "0x1234".to_string();
        let nonce = 0;
        let inbox_id = generate_inbox_id(&account_address, &nonce);

        let mut signature_request = IdentityUpdateBuilder::new(inbox_id)
            .create_inbox(account_address.into(), nonce)
            .add_association("0x5678".to_string().into(), account_address.into())
            .to_signature_request();

        for missing_signature in signature_request.missing_signatures() {
            signature_request.add_signature(somehow_get_signature_maybe_this_happens_over_ffi()).expect("should succeed")?;
        }

        let identity_update = signature_request.build_identity_update()?;

Copy link
Contributor Author

neekolas commented Apr 7, 2024

@neekolas neekolas marked this pull request as ready for review April 7, 2024 19:44
@neekolas neekolas requested a review from a team as a code owner April 7, 2024 19:44
@neekolas neekolas force-pushed the 04-05-add_identity_update_builder branch from 568a2cf to 4174ba6 Compare April 7, 2024 19:49
@neekolas neekolas force-pushed the 04-05-add_identity_update_builder branch 2 times, most recently from a93bfe6 to b4e75e4 Compare April 7, 2024 22:42
Base automatically changed from 04-05-add_unsigned_identity_updates to main April 8, 2024 16:09
@neekolas neekolas force-pushed the 04-05-add_identity_update_builder branch from b4e75e4 to 9c2569e Compare April 8, 2024 16:10
Copy link
Contributor Author

neekolas commented Apr 8, 2024

Merge activity

  • Apr 8, 12:10 PM EDT: Graphite rebased this pull request after merging its parent, because this pull request is set to merge when ready.
  • Apr 8, 6:57 PM EDT: @neekolas merged this pull request with Graphite.

self.signature_text.clone()
}

pub fn build_identity_update(&self) -> Result<IdentityUpdate, SignatureRequestError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we consume self here, or is there a benefit to keeping the intermediate builder around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I don't think you'll want the builder once this is done, so might as well consume self.

actions: Vec<PendingIdentityAction>,
}

impl IdentityUpdateBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to rename this SignatureRequestBuilder

Right now it looks like

let x = IdentityUpdateBuilder::new()

let x = x.to_signature_request()

x.build_identity_update()


But to me it makes more sense to build a SignatureRequest from a SignatureRequestBuilder which then resolves into a IdentityUpdate

like

let x = SignatureRequestBuilder::new()
let x = x.build()
/* platform SDK magic */
let identity_update = x.resolve()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's a good suggestion. I'll rename.

}

#[derive(Clone, PartialEq, Hash, Eq)]
enum SignatureField {
Copy link
Contributor

@insipx insipx Apr 8, 2024

Choose a reason for hiding this comment

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

The SignatureField enum here is a little confusing --

in IdentityUpdateBuilder we assign a PendingSignature with a SignatureField so that later in build_action we can retrieve the correct PendingSignature address/identity to compare it with the recovered signer from the given Signature.

I guess I'm ok with this, although i wish there was a more clear way to express/organize that code-wise. Will keep thinking about it, would be nice to have some docs explaining it on SignatureField though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree it's not the most intuitive. Batch signatures in general are a little confusing.

At the very least, I'll add some clarifying comments. And if we find a better way to structure the code we can refactor later.

Copy link
Contributor

@richardhuaaa richardhuaaa left a comment

Choose a reason for hiding this comment

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

This is a really elegant way to implement batch signatures. Seconding @insipx's comments to make it easier for others to understand.

Also, from the example code:

        for missing_signature in signature_request.missing_signatures() {
            signature_request.add_signature(somehow_get_signature_maybe_this_happens_over_ffi()).expect("should succeed")?;
        }

I think we can build higher-level API's over this before exposing to FFI. Rough sketch:

register_inbox_and_installation_key(Option<RecoveryAddress>): TextToSign; // installation key signature handled internally. Does not register inbox if inbox already exists
associate_external_wallet(): TextToSign; // installation key signature handled internally
... etc.

@neekolas neekolas merged commit 43884ec into main Apr 8, 2024
9 checks passed
@neekolas neekolas deleted the 04-05-add_identity_update_builder branch April 8, 2024 22:57
@richardhuaaa
Copy link
Contributor

Oof, didn't realize it was auto-merge @neekolas

@37ng 37ng mentioned this pull request Apr 10, 2024
@neekolas neekolas mentioned this pull request Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants