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

refactor!: signing and the Signer trait #1241

Merged
merged 27 commits into from Jan 15, 2024
Merged

Conversation

hal3e
Copy link
Contributor

@hal3e hal3e commented Dec 18, 2023

This PR refactors the Signer trait and the way we sign transactions.

The new Signer trait has two methods:

  • sign - signs a Message and returns a Signature asynchronously
  • address (public key)

When signing built Transactions we use the sign_with(& impl Signer, ChainId) method.
For TransactionBuilder we use the add_signer(impl Signer) method. The Signer (not the SecretKey as before) is stored on the heap and the sign method is called while building the tx.

In addition, this PR adds the ability to build transaction without signatures. This is useful if the user wants to estimate costs or wants to simulate the TX on a node without setting the right signatures.

Changes:

  • Refactor Signer trait
  • Added a new method called build_without_signatures(provider: &impl DryRunner) to TransactionBuilders
  • Replaced the Transaction check_without_signatures method with check
  • fee_checked_from_tx now builds without signatures which means we can use adjust_for_fee without signing first cc @MujkicA
  • Added documentation

NOTE: If the user builds without signatures, it is their responsibility to sign the transaction in the right order. For example, if we have CoinSignedA then CoinSignedB, we need to sign with WalletA and then with WalletB. This comes from the fact that we need to set the witness indexes while building the transaction.

BREAKING CHANGE:

  • Removed sign_message and sign_transaction from the Signer trait
  • Added sign and address methods to the Signer trait
  • Signer trait moved do fuels::core::traits:::Signer
  • Message, PublicKey, SecretKey and Signature moved to fuels::crypto::
  • Replaced Transaction's check_without_signatures with check
  • Renamed Accounts add_witnessses to add_witnesses
  • Removed Clone for TransactionBuilders

  • I have updated the documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary labels.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@hal3e hal3e added the enhancement New feature or request label Dec 18, 2023
@hal3e hal3e self-assigned this Dec 18, 2023
@hal3e hal3e changed the title feat!: add build_without_signatures to TransactionBuilder feat: add build_without_signatures to TransactionBuilder Dec 18, 2023
Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

Great work; very clean new API! Left some nits/suggestions.

docs/src/custom-transactions/transaction-builders.md Outdated Show resolved Hide resolved
docs/src/custom-transactions/transaction-builders.md Outdated Show resolved Hide resolved
packages/fuels-accounts/src/wallet.rs Outdated Show resolved Hide resolved
packages/fuels-core/src/types/transaction_builders.rs Outdated Show resolved Hide resolved
packages/fuels-core/src/types/transaction_builders.rs Outdated Show resolved Hide resolved
packages/fuels-core/src/types/transaction_builders.rs Outdated Show resolved Hide resolved
packages/fuels-core/src/types/transaction_builders.rs Outdated Show resolved Hide resolved
packages/fuels-core/src/types/transaction_builders.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@segfault-magnet segfault-magnet left a comment

Choose a reason for hiding this comment

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

Just some random thoughts that came with the morning coffee as I was reviewing this:

The names of sign_build_transaction and sign_transaction sound like we're injecting the fn argument type into the function name to workaround rust not having function overloading. Something akin to overloading can be achieved via traits. WDYT about having Signable and Signer traits (we already have the latter). Then have Signer sign anything that's Signable:

pub trait Signable {
    fn sign(&mut self, owner: &Bech32Address, key: &SecretKey, chain_id: ChainId) -> Result<()>;
}

Change the definitions of Transaction and TransactionBuilder traits to include the new trait as a bound:

pub trait Transaction:
    Signable + Into<FuelTransaction> + EstimablePredicates + GasValidation + Clone + Debug
// ...

pub trait TransactionBuilder: Signable + BuildableTransaction + Send + Clone {

Then implement for all tx types

        impl Signable for $wrapper {
            fn sign(
                &mut self,
                _owner: &Bech32Address,
                key: &::fuel_crypto::SecretKey,
                chain_id: ChainId,
            ) -> Result<()> {
                let id = self.id(chain_id);

                let message = Message::from_bytes(*id);
                let sig = Signature::sign(&key, &message);

                self.append_witness(sig.as_ref().into())?;

                Ok(())
            }
        }

and all tx builder types:

        impl Signable for $ty {
            fn sign(
                &mut self,
                owner: &Bech32Address,
                key: &SecretKey,
                _chain_id: ChainId,
            ) -> Result<()> {
                self.add_unresolved_signature(owner.clone(), key.clone());
                Ok(())
            }
        }

Then you can change Signer to have a single sign method that signs anything that's Signable:

pub trait Signer: std::fmt::Debug + Send + Sync {
    type Error: std::error::Error + Send + Sync;
    // ...
    fn sign<S: Signable>(&self, signable: &mut S) -> std::result::Result<(), Self::Error>;
}

When signing for both builders and transactions looks like this:

let tx = ...;
wallet.sign(&mut tx);

let tx_builder = ... ;
wallet.sign(&mut tx_builder); 

Some more morning thoughts:

Because of the whole witness index resolving thing we were forced to capture private keys until the tx is built and witnesses settled down (mostly).

This is why the Signable interface requires the private key. Ideally we should think about capturing a collection of Signer implementations to future-proof against hardware wallets as Signers -- in that case we won't be able to extract the private key -- we will be able only to provide a Message for the hardware wallet to sign.

So maybe prepare for that in the near future and have TxBuilders keep something like Vec<Box<dyn Signer>>.

Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

I 100% agree with @segfault-magnet; the Trait approach is really, really elegant. The means to it might not be as simple, but the end result is beautiful (the guy went out of his way to get his fn overload in Rust 😆).

Feeling neutral about his second point/thought, though. I'll think a bit more on it.

@hal3e hal3e changed the title feat: add build_without_signatures to TransactionBuilder refactor!: signing and the Signer trait Jan 8, 2024
@hal3e hal3e requested a review from digorithm January 9, 2024 13:03
digorithm
digorithm previously approved these changes Jan 9, 2024
MujkicA
MujkicA previously approved these changes Jan 10, 2024
Co-authored-by: MujkicA <32431923+MujkicA@users.noreply.github.com>
@hal3e hal3e dismissed stale reviews from MujkicA and digorithm via f46353f January 10, 2024 22:07
MujkicA
MujkicA previously approved these changes Jan 10, 2024
digorithm
digorithm previously approved these changes Jan 10, 2024
Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

:shipit:

Salka1988
Salka1988 previously approved these changes Jan 10, 2024
examples/predicates/Cargo.toml Outdated Show resolved Hide resolved
packages/fuels-core/src/traits/signer.rs Outdated Show resolved Hide resolved
packages/fuels-core/src/types/wrappers/transaction.rs Outdated Show resolved Hide resolved
@hal3e hal3e dismissed stale reviews from Salka1988, digorithm, and MujkicA via 7949ffd January 11, 2024 12:29
hal3e and others added 4 commits January 11, 2024 13:29
Co-authored-by: Ahmed Sagdati <37515857+segfault-magnet@users.noreply.github.com>
@hal3e hal3e merged commit bc32f7a into master Jan 15, 2024
40 checks passed
@hal3e hal3e deleted the hal3e/build-without-signatures branch January 15, 2024 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants