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

Support for EIP-2718 transaction envelope types beyond 1 and 2 #3747

Open
aaronmgdr opened this issue Feb 7, 2023 · 20 comments
Open

Support for EIP-2718 transaction envelope types beyond 1 and 2 #3747

aaronmgdr opened this issue Feb 7, 2023 · 20 comments
Labels
enhancement New feature or improvement. minor-bump Planned for the next minor version bump. on-deck This Enhancement or Bug is currently being worked on. v6 Issues regarding v6

Comments

@aaronmgdr
Copy link

aaronmgdr commented Feb 7, 2023

Describe the Feature

I would like to be able to send and sign transactions which have types beyond null 1 and 2 and which may contain additional parameters.

Currently Ethers does not allow transactions with other types and actively removes any unknown object parameters making the use of alternative transaction types impossible.

My initial use for this will be using Celo's 0x7c ie type 124 which adds support the gasFeeCurrency parameter to transactions.

//////

I plan to do the work for this. But want to start a discussion about how ethers would prefer to address this.

I could just submit a PR to allow the specific use case I have. Alternatively perhaps ethers could support some sort of bring your own Transaction Envelope parser. As of yet I don't know how it would look or if it would actually have benefits in code size and understandability.

Code Example

sendTransaction({
 type: 124,
 to: EOA_ADDRESS,
 from: EOA_ADDRESS,
 value: 0 
 feeCurrency: CUSD_CONTRACT_ADDRESS,
 //etc
})

Other cases where alternative Transaction details are being requested

@aaronmgdr aaronmgdr added the enhancement New feature or improvement. label Feb 7, 2023
@ricmoo
Copy link
Member

ricmoo commented Feb 7, 2023

My first thought would be to sub-class Transaction, and in the static from method, hijack the type you care about, creating a new CustomTransaction(params), which has all the extra properties you care about. And you can then override the *serialized methods, to use your serialization for your type, otherwise just call the super.

You can similarly override the inferTypes, clone and toJSON methods.

The built-in functions for parsing and serializing aren’t really exposed, but they are pretty trivial to work with using the decodeRlp and encodeRlp.

I’ve been considering adding custom network packages for non-standard networks. If you make any process, let me know and I can start that program earlier on. :)

@aaronmgdr
Copy link
Author

aaronmgdr commented Feb 7, 2023

Thanks for the quick reply.

I did some research a while back on Subclassing and as i went down the rabbit hole I kept needing to subclass and overwrite more methods. one issue was that copyRequest kept removing properties from the transaction object.

Im not sure what a moon-standard network is (although googling i learned The Moon is getting a G4 cellular network) I hope it means that the work im gonna do is helpful beyond my case

@ricmoo
Copy link
Member

ricmoo commented Feb 7, 2023

(updated: moon-standard -> non-standard)

Lol! Oh. Autocorrect.

I was contemplating the copyRequest function too, trying to think of a way to make it Network-specific or using the toJSON method if present and a function.

The reason I don’t want to ingrate anything too directly is that over the years, dozens of chains have wanted their “own logic” baked in. Many of those chains no longer exist (or I have never heard of them since). Some networks may use the same type for another format or such too. I’ve historically made separate packages for them, which various levels of success. For example, Aion required blake2b added because they use it for their hashes, which increases the library size for something no one else requires. Some networks required some large emscripten library of 1.2 megabytes of protocol buffers and whatnot just to compute the hash.

So, I target Ethereum and chains that don’t deviate too far. Part of v6 is to try making it easier to support non-standard networks (I like the term “strange networks”, meant as a fun term, but comes off more often as mean. :p)

So, I’m all ears if you have other suggestions.

I think if copyRequest works more universally, an otherwise unsupported chain shouldn’t require much more than a custom Transaction sub-class, a Network definition (possibly with custom NetworkPlugins) and possibly some custom Classes for the Provider objects (which the NetworkPlugins would coerce things into).

@aaronmgdr
Copy link
Author

Definitely respect that and that you have kept ethers as small as it can be.

Ill Do some more research in the direction of enabling easier extension rather than adding specific features

@ricmoo
Copy link
Member

ricmoo commented Feb 8, 2023

I’ll also add a feature in the v6.1 that makes it easier to support Transaction subclasses.

@ricmoo ricmoo added on-deck This Enhancement or Bug is currently being worked on. minor-bump Planned for the next minor version bump. v6 Issues regarding v6 labels Feb 8, 2023
@gastonponti
Copy link

What about adding a way to set delegators to handle different kind of txs. Something like, plug the type 0x14, and delegate it's calls to a specific class that has to cover the way you can serialize it, the extra data to be assigned and etc etc.
In a way every L2 or L1 ethereum like will need a way to create their own txs, maintaining the same compatibility with ethereum, but something like this could avoid projects to fork ethers, to instead plug their differentiators.
Ethersjs could be an agnostic library that others could also use, without forking and branching the community.

I'm not saying that you are the only ones that should do that, haha, but maybe a "could ethersjs accept to add a module that allow others to plug their txs without changing their code"

@aaronmgdr
Copy link
Author

I did a read thru of code for spots where there could be issues. #3751

Maybe its helpful maybe its not.

@aaronmgdr
Copy link
Author

aaronmgdr commented Feb 9, 2023

I was looking at the work done on Plugins and I think this could be a great way to handle extendability rather than Transaction subclasses as not every thing that needs to be extensible is part of that class and often most of the body for Transaction methods should remain.

export class TransactionEnvelopePlugin<TXType> extends NetworkPlugin {

    readonly eip2178type: number
    readonly serialize: (tx: TXType, sig?: Signature) => string
    readonly parse: (tx: Uint8Array) => TXType
    readonly extraTXProperties: string[]

    constructor(
        eip2178type: number,
        serialize: (tx: TXType, sig?: Signature) => string,
        parse: (tx: Uint8Array) => TXType,
        extraTXProperties: string[] // properties in the tx obj this type supports beyond the ones supported in type 1 and 2
        // alternatively a copyRequest function. 
    ) {
        // should the type be part of name?
        super("org.ethers.plugins.network.txe");

        this.eip2178type = eip2178type
        this.serialize = serialize
        this.parse = parse
        this.extraTXProperties = extraTXProperties
    }

    clone(): TransactionEnvelopePlugin<TXType> {
        return new TransactionEnvelopePlugin(this.eip2178type, this.serialize, this.parse, this.extraTXProperties);
    }

}

@ricmoo
Copy link
Member

ricmoo commented Feb 9, 2023

The problem is Plugins are mainly asynchronous in nature. And the Transaction operations are all synchronous. You don't always have a Network object, but should still be able to process Transactions...

But I was imagining, for a given network it would have code that looks like:

import { ethers } from "ethers";

// This Transaction class is a sub-class of ethers.Transaction, and should be used when the
// envelope types are necessary
import { CeloNetwork, Transaction } from "@ethers-ext/celo";

// The CeloNetwork would certainly have a bunch of plugins though, to handle custom
// block properties, transactions, etc.

const provider = new JsonRpcProvider(url, CeloNetwork);

The problem with global registration is that various bundlers and other tools really do not play well with global registration things. Especially if there are 2 different versions of ethers in use (such as when using hardhat for testing), as the registration only happens against the version where the registration happened explicitly, which then means it either works in the app and not in the tests or works in the tests and not in the app.

A bigger issue, is that it is complicated to use in ESM projects, since import is always async, things like this are confusing and really don't work how people expect them to:

// SetupCelo/index.js
import { ethers } from "ethers"; 
ethers.Network.networks.celo = "foobar";


// main.js
import "SetupCelo"
import { ethers } from "ethers";

// Things done here operate before all imports are SetupCelo is resolved
console.log(ethers.Network.networks.celo);
// undefined

setTimeout(() => {
  // Now things are fine, but developers are (justifiable) confused... And also have to wrap
  //  all their code to execute in the event loop
  console.log(ethers.Network.networks.celo);
  // "foobar"
}, 0);

There are a lot of little gotchas, that have gotcha-ed me over the years. :)

@aaronmgdr
Copy link
Author

Thanks for talking this out. 😊

I agree its best to avoid global registration.

The problem is Plugins are mainly asynchronous in nature. And the Transaction operations are all synchronous.

Maybe I just haven't understood how you intend for plugins to be used but I don't see anything inherently async about them.

I saw AbstractProvider has access to a attachPlugin function. I realized later that ProviderPlugins are different than NetworkPlugins. But had been thinking the TransactionEnvelopePlugin would be attached to the provider and then since signers have access to provider either the entire class or the relevant method could be passed as an optional argument to functions like populate. and copyRequest

Probably has issues still. 🤔

Do you think the idea of some object that given a tx type number can say ok "parse with this, serialize with this, these are the allowed/needed properties" is a good way to encapsulate allowing alternative tx types? ?

@dckesler
Copy link

dckesler commented Mar 7, 2023

I noticed that this issue was labeled with on-deck and I was wondering given this comment

I’ll also add a feature in the v6.1 that makes it easier to support Transaction subclasses

and with the recent release of v6.1.0 if there were any updates on this issue?

@ricmoo
Copy link
Member

ricmoo commented Mar 8, 2023

I implemented it the way I was originally planning and had to back it out because there were still some underlying issues with it, that make it more complex than I expected.

It’s going to need NetworkPlugin support and more fine grained changes I need to experiment with, so it didn’t make it into v6.1.

It’s still on my priority list though, which is why it’s still on-deck. ;)

@hill399
Copy link

hill399 commented Sep 11, 2023

Hey, has there been any further updates on the above?

@ricmoo
Copy link
Member

ricmoo commented Sep 11, 2023

Not recently, sorry. I’m still experimenting with the best way to add the functionality as a NetworkPlugin. I am doing a next-patch blitz this week, which means I’ll be revisiting the on-deck soon too. I may rename these tags as they are starting to get confusing as to their original meanings…

@Sledro
Copy link

Sledro commented Sep 13, 2023

+1

would also love to see EIP-2718 supported. Currently exploring alternative options.

@arthurgousset
Copy link

arthurgousset commented Oct 24, 2023

Check-in

Hey @ricmoo :) Brief follow up, are there any recent changes that would make it easier to support chain-specific transactions types in ethers v6? If not, is there anything we can do to help?

Thanks in advance!

Some history and context (in case it's useful)

In June 2023, we (Celo engineers from cLabs 👋) proposed a design that would allow viem to support chain-specific transaction serializers in this PR:

The PR was kindly accepted, and we were the first team to implement a chain-specific transaction type in viem:

Since then, Optimism transaction types were also added:

We repeatedly hear from developers that they would love to build applications with ethers and access to "custom gas currency transactions" supported on Celo. As far as we know, this is not possible so, unfortunately, we resorted to wrapping ethers and re-implementing a chunk of the library in the past to support custom transaction types: celo-ethers-wrapper.

@ricmoo
Copy link
Member

ricmoo commented Nov 2, 2023

I had an idea on how to do this the other day, and will likely try an experimental branch to test it out, but curious on your thoughts.

What if the type of a transaction, which today is a Numeric type was instead Numeric | (t: TransactionLike) => Transaction? This would allow for custom transaction types as a function, which accepts the transaction and is responsible for creating the necessary serialized formats (for both signed and unsigned).

This would allow Celo, for example to create a GasCurrencyType (or some similar name) function which could be passed in like: signer.sendTransaction({ to, value, type: GasCurrencyType }); the signer internally, if the .type is a function would call that instead of the current default which is to use Transaction.from.

The function would returns a sub-class of Transaction, extended to support the additional transaction type.

Feedback? :)

@shazarre
Copy link

shazarre commented Nov 8, 2023

Thanks @ricmoo so much for putting your effort into it! Your approach sounds really good. Just couple of clarifying questions:

  • how would extra fields be handled?
  • would the function not only serialize but also add feeCurrency itself?

I briefly tried it and seems to me that (given my limited understanding) changing the type could be a bit difficult though as this particular field is used in so many places (while populating transaction fields for example) and type checks would have to be introduced in all of those places.

My suggestion would be to introduce a new field (transactionFactory?) to TransactionRequest instead of changing type of the type field. I think it could just simplify it.

So let's say we would introduce a new transaction (just a descriptive example):

class DynamicFeeCurrencyTransaction extends Transaction {
    protected feeCurrency : string;

    constructor(feeCurrency: string) {
        super();

        this.feeCurrency = feeCurrency;
    }

    get type(): null | number { 
        return 123; 
    }

    get typeName(): null | string {
        return "cip64";
    }

    get serialized(): string {
        return '0xsigned';
    }

    get unsignedSerialized(): string {
        return '0xunsigned';
    }
}

and then provide the transactionFactory as mentioned above that would actually create an instance of the transaction:

const tx = await signer.sendTransaction({
    type: 123,
    to: "0xaddress",
    value: 1n,
    transactionFactory: (tx: TransactionLike): Transaction => {
        const result = new DynamicFeeCurrencyTransaction("0x874069Fa1Eb16D44d622F2e0Ca25eeA172369bC1");

        // populate all fields from tx object

        return result;
    }
});

Then AbstractSigner.sendTransaction could instead of calling Transaction.from() do:

typeof tx.transactionFactory === "function" 
    ? tx.transactionFactory(pop) 
    : Transaction.from(pop);

but my issue is that there's another call to Transaction.from in BaseWallet.signTransaction and there the transactionFactory value is lost due to conversion between TransactionReqest > TransactionLike > Transaction, so somehow the value would have to be propagated througout.

No idea yet for signing (but I can imagine something similar could work here?).

What do you think about that approach?

@shazarre
Copy link

We’d like to bring some extra attention to this discussion with the aim of getting a solid plan in place.

Why do we think supporting fee currency feature is important in general?

Custom gas fee currency feature allows projects and developers on Celo network to use whitelisted tokens to be used to pay for gas. It provides flexibility and seamless experience by not requiring users to hold native currency just to be able to pay for the network fees.

While the feature has been there for a while and there are examples of successful projects using it like Valora (code example) and Impact Market (code example).

MiniPay supports the feature as well and by reaching a 1 million users milestone it plays a huge role in adoption.

It also has gained more traction recently with projects like GoodDollar (see discussion) and USDC (see discussion) are on the radar to possibly become gas currencies as well.

Why do we think supporting the custom transaction serialization is important for ethers?

In the ethereum realm, from the standard ethers/viem/web3 libraries, ethers is currently #1 library (based on the trends).

If someone were to start a new multichain (or chain specific) project though they might ask themselves a question “How many chains can I support using a single library?” and this is in our opinion ethers comes short. We believe that extensibility such as support for custom transactions will simply help ethers to maintain its #1 spot.

viem is a very good example of the utility of extensibility. Since the introduction of custom serialisers for Celo, there are now many other chains with custom extensions officially featured in the library. The OP stack extension for example furthers Ethereum’s L2 vision by providing a serializer for L2 deposit transactions

In contrast to the experience in viem, providing support for other transaction types for ethers requires modifying several classes from ethers in a brittle way. For celo-ethers-wrapper we have found this is difficult to maintain, violates SOLID principles, and is a sore point for developers.

And celo community developers have shown their desire for support for fee currencies in ethers already.

How to make it happen?

Based on the discussion (+ few more) there are multiple ways how to go forward:

  • subclass Transaction class
  • pass a custom function/class via transaction object
  • use NetworkPlugin architecture to "intercept" transactions and allow for custom serialization
  • inject Network class to signers/providers which would encapsulate custom signing/serializing logic
  • introduce top-level chain-specific serializers
  • allow chain-specific logic inside existing providers/signers

or combination of some/all of the above.

We’re more than happy to provide code snippets, create a branch with some ideas implemented, collaborate on making this happen, but first we’d love to hear your feedback and agree on the plan on how to make it happen.

Thank you for your support for this initiative and engagement into the discussion so far!

@sirpy
Copy link

sirpy commented Feb 18, 2024

I General I think libraries for protocols should have easy ways to extend the protocol they support, usually by simply exposing the relevant fields as public and writable, seems like the easiest route.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement. minor-bump Planned for the next minor version bump. on-deck This Enhancement or Bug is currently being worked on. v6 Issues regarding v6
Projects
None yet
Development

No branches or pull requests

9 participants