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

feat(consensus): signer field in Signed tx. #484

Closed
wants to merge 2 commits into from

Conversation

yash-atreya
Copy link
Member

Motivation

Ref #481

Solution

Added signer: OnceLock<Address> in Signed

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

#[cfg(not(feature = "no_std"))]
if let Some(signer) = self.signer() {
return Ok(*signer);
}
Copy link
Member

@prestwich prestwich Apr 11, 2024

Choose a reason for hiding this comment

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

proposed api

  • delete recover_signer as standard API
  • add new_with_unchecked_signer for instantiation when signer is known (e.g. when we just signed)
  • modify signer as follows:

/// get the signer if known or recoverable
fn signer(&self) -> Option<Address> {
    #[cfg(not(feature = "k256"))]
    self.signer.get().copied()

    #[cfg(feature = "k256")]
    *self.signer.get_or_init(|| { /* recovery logic here */ })
}

Copy link
Member

Choose a reason for hiding this comment

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

also ok if implemented as

#[cfg(not(feature = "k256"))]
fn signer(&self) -> Option<Address> { ... }

#[cfg(feature = "k256")]
fn signer(&self) -> Option<Address> { ... }

whatever feels cleaner

@prestwich
Copy link
Member

I did a little bit of work here, and because we're attempting to be generic over Sig, this is harder than it seems without a Recoverable trait for the signature type. which i'm not sure we want to introduce yet. Going to close this as needing more design work

another problem specialization would solve 🤔

@prestwich prestwich closed this Apr 16, 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.

2 participants