Skip to content

feat(consensus): TypedTransaction#183

Closed
Evalir wants to merge 5 commits intomainfrom
evalir/add-typed-tx-request
Closed

feat(consensus): TypedTransaction#183
Evalir wants to merge 5 commits intomainfrom
evalir/add-typed-tx-request

Conversation

@Evalir
Copy link
Contributor

@Evalir Evalir commented Feb 5, 2024

Motivation

There is no proper concrete type which contains all ethereum transaction types implemented (legacy/eip2930/eip1559), which can be used by a alloy-providers or alloy-contract for remote/local signing and sending txs to the network. We have TransactionRequest/CallRequest, but it is an RPC type, and does not support encoding / signing.

Solution

Adds TypedTransaction to alloy-consensus—an enum which represents the unsigned version of TxEnvelope, which can contain any of the ethereum transaction types.

The rationale behind this type is for it to be the built-in concrete type used in providers to either use for remote account signing (send_transaction), or for signing locally and sending through the network (send_raw_transaction).

In the case of send_raw_transaction, it is expected for the TypedTransaction to be signed, and to then be converted into a TxEnvelope through to_enveloped, which can then be encoded and sent through the network. Decoding is not implemented, as this type is not meant be used in this manner.

With #178, we should also be able to implement a conversion from TransactionRequest -> TypedTransaction, which currently exists in ethers with into_typed_tx.

The enum implements Transaction, meaning it is signable by the alloy signers.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@Evalir Evalir requested review from mattsse and onbjerg February 5, 2024 13:09
@Evalir Evalir marked this pull request as ready for review February 5, 2024 13:54
@Evalir Evalir mentioned this pull request Feb 5, 2024
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

hmm, is this not an rpc type? alternatively, is this even a TransactionRequest type? is it not just TypedTransaction? common for the TransactionRequest types is that almost all the fields are optional

basically, does this belong in the consensus crate?

@Evalir
Copy link
Contributor Author

Evalir commented Feb 5, 2024

Hmm, I don't think this should be an RPC type? How I see it is that:

  1. TransactionRequest, the one being deduped on refactor: dedupe CallRequest/TransactionRequest #178, is the corresponding RPC type.
  2. TransactionRequest does not implement any encoding functionality for signing. We don't have any concrete type we can pass to the provider to sign, other than passing specifically a TxLegacy/TxEip2930/TxEip1559. This enum is the equivalent of https://github.com/gakonst/ethers-rs/blob/master/ethers-core/src/types/transaction/eip2718.rs#L19-L47, which can be signed.
  3. The idea is that TransactionRequest can be transformed into this type by doing into_typed_request, which we'd need to implement on TransactionRequest.

I'm not sure we're on the same page on how we'd sign transactions on the provider without this type—could you please elaborate @onbjerg ?

I think we could also follow reth's approach, and keep this TypedTransaction as an RPC type, but be able to convert it into an alloy-consensus transaction, like so: https://github.com/paradigmxyz/reth/blob/main/crates/rpc/rpc-types-compat/src/transaction/typed.rs#L7-L58. wdyt @mattsse ?

@Evalir Evalir force-pushed the evalir/add-typed-tx-request branch from 2742961 to 82db26e Compare February 5, 2024 14:50
@Evalir Evalir changed the title feat(consensus): TypedTransactionRequest feat(consensus): TypedTransaction Feb 5, 2024
@Evalir Evalir requested review from mattsse and onbjerg February 5, 2024 14:51
@mattsse
Copy link
Member

mattsse commented Feb 5, 2024

I think we could also follow reth's approach, and keep this TypedTransaction as an RPC type, but be able to convert it into an alloy-consensus transaction, like so: https://github.com/paradigmxyz/reth/blob/main/crates/rpc/rpc-types-compat/src/transaction/typed.rs#L7-L58. wdyt @mattsse ?

this is an intermediary step that might no longer be required, need to revisit this again

but @onbjerg the general idea is

you start out with a general purpose CallRequest, convert that into a signable TypeTransaction but setting all the required fields and then this type is signable

Comment on lines +262 to +265
async fn sign_dyn_tx_test(tx: &mut SignableTx, chain_id: Option<ChainId>) -> Result<Signature> {
let mut wallet: Wallet<SigningKey> =
"4c0883a69102937d6231471b5dbb6204fe5129617082792ae468d01a3f362318".parse().unwrap();
wallet.set_chain_id(chain_id);
Copy link
Member

Choose a reason for hiding this comment

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

Can we make it not require the SigningKey generic? We had type LocalWallet for that before.

Comment on lines +153 to +159

fn into_signed(self, _signature: Signature) -> alloy_network::Signed<Self, Self::Signature>
where
Self: Sized,
{
unimplemented!();
}
Copy link
Member

Choose a reason for hiding this comment

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

What do we need here?

Copy link
Member

Choose a reason for hiding this comment

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

ye this should be implable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is, but it is probably not what you want, right? As this would return a Signed<TypedTransaction>, and what you actually want to do is extract the actual variant contained, use into_signed on it and get a Signed<TxLegacy/TxEip2930/TxEip1559>.

where
Self: Sized,
{
unimplemented!();
Copy link
Member

Choose a reason for hiding this comment

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

same q

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same reasoning as above—you want to decode a signed enveloped tx with TxEnvelope, not with this type

@Evalir
Copy link
Contributor Author

Evalir commented Feb 5, 2024

Actually after some thought about how to design this I find myself liking the reth approach a bit more though @mattsse .

Having TypedTransaction as an RPC type instead of a consensus type, means we can go from TransactionRequest to TypedTransaction easily with a conversion. To go from TypedTransaction to an actual consensus transaction e.g when signing, we can simply convert to the contained variant with a helper function e.g .into_consensus_tx, which will result in an alloy-consensus tx type which implements Transaction , and is signable. This means i slide a bit more with @Bjerg 's original criticisms on the pr.

I think this is nice because:
Adding different TypedTransaction impls (e.g for having one which also include optimism deposit txs) is quite easier if they're RPC versus consensus.
Instead of having to go from TypedTransaction -> TxEnvelope and then extracting the variant, and signing that, we can just get the actual tx to be signed, with .into_consensus_tx.
We won't need to implement the Transaction trait over this type (TypedTransaction), but rather delegate that to the type that will be returned by .into_consensus_tx.

EDIT: Discussed offline, disregard. Reth also needs to be updated to a newer approach

@prestwich
Copy link
Member

i have some strong feelings here, obv. TypedTransaction doesn't make sense as a type. It's the unhappy middle between consensus format TransactionEnvelope and RPC format. What does make sense is enum TransactionResponse, and some enum TransactionRequest

@prestwich
Copy link
Member

even then, the TransactionRequest should instead be a TransactionBuilder that is used to construct EITHER OF TxEnvelope or a json request

@Evalir
Copy link
Contributor Author

Evalir commented Feb 5, 2024

@prestwich i think part of the problem is naming IMO. Reth calls this type TypedTransactionRequest, while ethers calls it TypedTransaction. This is a typed transaction request, meaning: it has disambiguated which type of tx this is, with the fields assigned, and we want to sign this type.

even then, the TransactionRequest should instead be a TransactionBuilder that is used to construct EITHER OF TxEnvelope or a json request

Ok, and how do we go from the json request to signing? This is the part that i think we are not thinking clearly about. At some point, this needs to become a signable tx, no?

@prestwich
Copy link
Member

prestwich commented Feb 5, 2024

This is a typed transaction request, meaning: it has disambiguated which type of tx this is, with the fields assigned, and we want to sign this type.

This is the sticking point. We don't sign TransactionRequest. We sign specific types T: Transaction. TransactionRequest is submitted to the server via RPC. We need to provide a TransactionBuilder that can be built into either TransactionRequest or some set of T: Transaction that can be signed and be put in a TransactionEnvelope

basically

trait TxBuilder<N> {
    fn gas(mut self, gas: U256) -> Self;
    fn data(mut self, data: Bytes) -> Self;

    // etc

    fn build_req(self) -> N::TransactionRequest;
    fn sign<S>(self, signer: S) -> Result<N::TransactionEnvelope>;
}

@prestwich
Copy link
Member

and our signing needs to be flexible enough to all non-secp256k1 sigs :)

@Bjerg

This comment was marked as spam.

@Evalir
Copy link
Contributor Author

Evalir commented Feb 5, 2024

This is the sticking point. We don't sign TransactionRequest. We sign specific types T: Transaction.

Agreed, and here's how I view this:

  • We have TransactionRequest. See refactor: dedupe CallRequest/TransactionRequest #178.
  • TypedTransaction by itself is not signable, but rather delegates signing to its contained variants (TxLegacy/TxEip1559/TxEip2930). I agree TypedTransaction itself implementing Transaction looks weird, but we can always just manually extract the variant and get the concrete tx type to sign if that looks better.

Here's how I'm thinking about the full interaction:

An user uses alloy-contract to build a tx (or manually with TransactionRequest). Alloy contract will return TransactionRequest. A signer cannot accept TransactionRequest, as it needs an specific type T: Transaction. Therefore, before signing, we need to disambiguate what we're signing. TypedTransaction serves as this type, as TransactionRequest will be able to go from TransactionRequest -> TypedTransaction. For signing, then we take the contained tx in TypedTransaction, and sign that, put it in a TxEnvelope, and finally send it over the network with a provider.

Therefore, the interaction would look like:

  1. alloy-contract builds a TransactionRequest
  2. Convert that TransactionRequest into a TypedTransaction
  3. Extract the concrete tx to sign with TypedTransaction.tx(), and sign.
  4. Put the signed tx in a TxEnvelope, and send over the network.

We need to provide a TransactionBuilder that can be built into either TransactionRequest or some set of T: Transaction

This is what alloy-contract would be for—the tx can also be built manually with the TransactionRequest type. We can discuss more call builders but I think this is also something users can implement on their own.

I think we'll probably need to diagram this, as I think we're running the risk of making this more complex than it needs to be.

@prestwich
Copy link
Member

we're once again running ahead of ourselves by building alloy-contract before we have a full Tx model 😮‍💨

@mattsse
Copy link
Member

mattsse commented Feb 5, 2024

This is the sticking point. We don't sign TransactionRequest. We sign specific types T: Transaction. TransactionRequest is submitted to the server via RPC. We need to provide a TransactionBuilder that can be built into either TransactionRequest or some set of T: Transaction that can be signed and be put in a TransactionEnvelope

fn build_req(self) -> N::TransactionRequest;

yeah so the main (unresolved) question is what's the TransactionRequest here

TransactionRequest is submitted to the server via RPC.

this applies to all endpoints besides eth_sendRawtransaction for which we need the TransactionEnvelope

The distinction between TransactionRequest and T: Transaction is important:

  • TransactionRequest is just the RPC type
  • T: Transaction is a signable representation of a (typed) transaction

A TransactionRequest can be converted into T: Transaction by setting all required fields

what this PR adds is the T: Transaction for ethereum (minus EIP-4844)

This trait:

trait TxBuilder<N> {
    fn gas(mut self, gas: U256) -> Self;
    fn data(mut self, data: Bytes) -> Self;

    // etc

    fn build_req(self) -> N::TransactionRequest;
    fn sign<S>(self, signer: S) -> Result<N::TransactionEnvelope>;
}

could be implemented on TransactionRequest (== N::TransactionRequest), although for ergonomic reasons perhaps we need a different builder type

but the conversion from TransactionRequest -> TypedTransaction is relevant in sign

. TypedTransaction doesn't make sense as a type. It's the unhappy middle between consensus format TransactionEnvelope and RPC format.

I think it's necessary because we want a clean way to get the signature hash we're signing. This step is just easier if we have a typed representation of the transaction

@Evalir
Copy link
Contributor Author

Evalir commented Feb 5, 2024

what this PR adds is the T: Transaction for ethereum (minus EIP-4844)

note that when i rebase i'll add eip-4844 too

@prestwich
Copy link
Member

prestwich commented Feb 5, 2024

The distinction between TransactionRequest and T: Transaction is important:
TransactionRequest is just the RPC type
T: Transaction is a signable representation of a (typed) transaction

We don't want a signable representation of a typed transaction. We want a builder that either (coordinates signing and constructs a tx envelope) or (outputs a json request object). The builder should not impl Transaction, as it is not a transaction

@mattsse
Copy link
Member

mattsse commented Feb 5, 2024

yeah, my point is that this step

coordinates signing and constructs a tx envelope

is easier if you have the actual transaction types, which will happen inside sign or before that (hidden,internally) in some builder function

@prestwich
Copy link
Member

prestwich commented Feb 5, 2024

yeah, my point is that this step
coordinates signing and constructs a tx envelope
is easier if you have the actual transaction types, which will happen inside sign or before that (hidden,internally) in some builder function

The builder needs type knowledge of the inner transactions so it can convert itself into them during the envelope instantiation process, but almost all other parts of the API should never have that. They should operate only on builders and envelopes. It is a mistake to make a pub enum TypedTransaction

@Evalir
Copy link
Contributor Author

Evalir commented Feb 5, 2024

I'd like to agree on a direction soon—this is blocking for further foundry work.

The builder needs type knowledge of the inner transactions so it can convert itself into them during the envelope instantiation process

how would this look? and in the case of sending an unsigned transaction, how would we disambiguate between the different transaction types properly? I'm struggling to look how this would work without this TypedTransaction type that coordinates signing and constructing the envelope.

@Evalir
Copy link
Contributor Author

Evalir commented Feb 6, 2024

ping @mattsse / @prestwich

@onbjerg
Copy link
Member

onbjerg commented Feb 6, 2024

This is possibly also blocking paradigmxyz/reth#6364 since a typed tx repr is used in reth, and that PR is a prerequisite for #178 getting merged which is a prerequisite for me being able to rename the provider and add a network generic for both the provider and CallBuilder in alloy-contract 🥵

@prestwich
Copy link
Member

then we should really make sure we get this right before it gets into the internals of everything 😅

what would you think about having the builder be RPC request format? I.e. reth desers a builder

@prestwich
Copy link
Member

@Evalir i'm gonna noodle around with this today and try to get things unblocked

@Evalir
Copy link
Contributor Author

Evalir commented Feb 7, 2024

Closing in favor of #190 per telegram discussion.

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.

6 participants