-
Notifications
You must be signed in to change notification settings - Fork 245
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
Feat: allow multiple signing credentials in NetworkSigner #515
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.
I like this, this is pretty much exactly what we have here:
can we also add, or similar:
/// Returns `true` whether this signer can sign for this address
fn is_signer_for(&self, addr: &Address) -> bool
/// Asynchronously sign an unsigned transaction, with a specified | ||
/// credential. | ||
async fn sign_transaction_from( | ||
&self, | ||
sender: Option<Address>, | ||
tx: N::UnsignedTx, | ||
) -> alloy_signer::Result<N::TxEnvelope>; |
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.
this makes sense to me and is convenient to have,
it should be trivial for any, non-multiplex signers to implement by ensuring the default signer is Some(sender)
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.
doing a slight cleanup here as i wrote this function signature before i added the default signer
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.
removed the Option
because we can rely on the caller to call default_signer
Added that, and a method to get an iterator of addresses that we can sign with |
@@ -148,7 +148,7 @@ impl TransactionBuilder<Ethereum> for TransactionRequest { | |||
self, | |||
signer: &S, | |||
) -> BuilderResult<<Ethereum as Network>::TxEnvelope> { | |||
Ok(signer.sign_transaction(self.build_unsigned()?).await?) | |||
Ok(signer.sign_request(self).await?) |
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.
these were dropping the from
field during the build
process regardless of whether it were set in the first place 😓
What does "Preswitch/signer multiplex" mean? 😄 |
branch name 😅 |
* feature: signer multiplexing * fix: register default on instantiation * docs: fixup * fix: clippy * fix: sign_request * fix: clippy * feat: is_signer_for and signers * fix: signer returns copied addresses * fix: remove unnecessary option
Resolves signer filler not setting from and failing to sign
Motivation
Unblock foundry dev
Solution
NetworkSigner<N>
is now expected to contain a store of credentials, by address.NetworkSigner<N>
now selects credentials based on requestfrom
fieldNetworkSigner<N>
allows user-specified sender when signingPR Checklist