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

Signature verification requests routing #1094

Merged
merged 18 commits into from
Oct 3, 2024
Merged

Signature verification requests routing #1094

merged 18 commits into from
Oct 3, 2024

Conversation

nplasterer
Copy link
Contributor

@nplasterer nplasterer commented Sep 26, 2024

"Create MultiChainSignatureVerifier that routes signature verification requests to RpcSmartContractWalletVerifier instances configured with a RPC URL for the correct chain."

Part of #1070

Depends on xmtp/proto#211

codabrink and others added 10 commits September 24, 2024 12:41
* imlement SmartContractVerification for lots of chainids

* remove comment
* move file parser

* load in verifier

* cleanup

* cleanup

* log malformatted url

* chainurls.json

* closure, dont expect

* go gentle into that good night

* default

* lint

* cleanup
* block number optional
@nplasterer nplasterer assigned nplasterer and unassigned nplasterer Sep 26, 2024
Copy link

graphite-app bot commented Sep 26, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@nplasterer nplasterer marked this pull request as ready for review September 26, 2024 14:47
@nplasterer nplasterer requested a review from a team as a code owner September 26, 2024 14:47
xmtp_id/src/lib.rs Outdated Show resolved Hide resolved
xmtp_mls/src/builder.rs Outdated Show resolved Hide resolved
@insipx
Copy link
Contributor

insipx commented Sep 26, 2024

Before merging should:

  • clean up the Box<dyn SmartContractVerifierTrait> (Clone impl, where on client its stored, etc.)
  • write a test maybe
  • dont forget about this line

* wip

* wip

* patchwork

* a few fixes

* update to use scw verifier for inbox_ids

* Fix build errors

* fix signature

* lint

* remove debug

* lint

* revert gen protos script

* restore tests too

* feedback

* cleanup, feedback

* cleanup

* cleanup

* hash

* Actually do the work, and take the hash from the proto

* cleanup

* more cleanup

* not using error msg

* use latest proto

* wip

* cleanup

* nicer error

* revert

* unnecessary change

* remove env logger

* cleanup

* false in place of none

* lint
@codabrink
Copy link
Contributor

Depends on xmtp/proto#211

Copy link
Contributor

@insipx insipx 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 except for a comment on the try_join!

responses.push(handle);
}

let responses: Vec<_> = try_join_all(responses)
Copy link
Contributor

Choose a reason for hiding this comment

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

We might need to be careful here -- try_join_all will abort the entire operation if only one of the futures fail. It might be better to use join_all here, emit a log if one of the futures fail and transform that failed future to is_valid: false

Copy link
Contributor

@insipx insipx Oct 3, 2024

Choose a reason for hiding this comment

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

Could also return a Vec<> of errors or something. Might be ok to do it in followup PR as long as an issue is created

Copy link
Contributor

Choose a reason for hiding this comment

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

Giving that a little more thought, I should add an error message field to the validation response, so this doesn't turn into a black box. Let me get a quick proto pr up.

Copy link
Contributor

Choose a reason for hiding this comment

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

@codabrink codabrink merged commit 2acfde8 into main Oct 3, 2024
9 checks passed
@codabrink codabrink deleted the offsite/scw-libxmtp branch October 3, 2024 17:08
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.

4 participants