Tx: New supports(capability) method

[holgerd77]

Fixes #1242 respectively addresses #1313

Ok, this is now addressing the tx.transactionType, tx.type and - especially tx instance of comparison issue (browser compatibility & being future proof on feature vs tx type comparison).

I thought a bit longer about comparing with enum strings vs. comparing with EIP numbers. At the end I gave comparing with EIP numbers a preference since this aligns relatively well with existing Common syntax on lines like if (opts.tx.supportsEIP(2718) && this._common.isActivatedEIP(2718)) { and I personally find this simpler for this very much limited set of EIPs (which will also likely grow very slowly) since one is avoiding an additional import. Last reason: I am still not so sure if JavaScript works so well with enums (but was also too lazy to properly look this up TBH since I already had these previously stated reasons).

@ryanio let me know if you go along, we can also still change with a limited amount of additional work for changing.

Still WIP, need to replace some more occurrences, just wanted to push early to see if tests - especially on the VM - will pass.

[codecov]

Codecov Report

Merging #1315 (918724a) into master (6ab249c) will decrease coverage by 0.16%.
The diff coverage is 88.00%.

Flag	Coverage Δ
block	86.20% <50.00%> (ø)
blockchain	83.39% <ø> (-0.07%)	⬇️
client	83.66% <ø> (-0.84%)	⬇️
common	88.70% <ø> (ø)
devp2p	84.00% <ø> (ø)
ethash	82.83% <ø> (ø)
tx	88.36% <94.44%> (-1.13%)	⬇️
vm	79.23% <75.00%> (+0.20%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

[holgerd77]

I have simplified here since I think this is not very future proof if we bring out some new tx type within a new non-breaking tx release and this would be run on a new VM (this would then break here without the change) and at the same time I am not really seeing the use for these dedicated receipt types.

Hope that I am not overlooking some side effects here though, eventually also give this some additional thought if you think this is ok.

[holgerd77]

(and if there will be some future tx type changing the receipt format once again we can still then think about how we react and eventually add a specific new receipt type here)

[ryanio]

I believe the reason this was implemented with dedicated receipt names for each EIP is due to EIP-2718 saying:

• Receipt is either TransactionType || ReceiptPayload or LegacyReceipt
• ReceiptPayload is an opaque byte array whose interpretation is dependent on the TransactionType and defined in future EIPs
• LegacyReceipt is rlp([status, cumulativeGasUsed, logsBloom, logs])

EIP-2930:

The EIP-2718 ReceiptPayload for this transaction is rlp([status, cumulativeGasUsed, logsBloom, logs]).

EIP-1559:

The EIP-2718 ReceiptPayload for this transaction is rlp([status, cumulativeGasUsed, logsBloom, logs]).

So although the receipts are aligned so far, they might deviate in the future. That said, I think your code improvements and reasoning here are great and we can introduce a new receipt type when the time comes.

[holgerd77]

Ok, this is now ready for review. 😄

[acolytec3]

My one thought here is do we need both supportsEIPs and common.isActivatedEIP available on the transaction object? Feels duplicative to have both. If a transaction supports EIP whatever, shouldn't it also be activated on this.common where this is a transaction object? It seems to me a better solution would be to just have just one method that provides these details.

[holgerd77]

My one thought here is do we need both supportsEIPs and common.isActivatedEIP available on the transaction object? Feels duplicative to have both. If a transaction supports EIP whatever, shouldn't it also be activated on this.common where this is a transaction object? It seems to me a better solution would be to just have just one method that provides these details.

Yes, I first (or actually: on a second or third iteration 😋) had this thought too: couldn't we just use the information we already have in the Common. But actually we can not, this is something different. While common reflects the network state, this method is dedicatedly targeted to describe the capabilities of a specific tx type. This becomes apparent if you take a legacy tx running on london. So this is running on a network with 1559 enabled (and so: common.isActivatedEIP(1559) will return true) while the tx itself has no 1559 capabilities.

[acolytec3]

Got it. That makes sense now that you explain it. I'm approving though don't take that as 100% guarantee that it should be merged. I'll defer to @ryanio for a knowledgeable review.

[gabrocheleau]

It might be interesting to consider turning the supportsEIP method into a TypeScript typeguard. This would allow automatic inference of the transaction type (no as FeeMarketEIP1559Transaction would then be necessary).

[ryanio]

Oh that's a really cool idea, could you commit it to this branch or sketch out how we may do that? I am just reading about it, super interesting: https://www.typescriptlang.org/docs/handbook/advanced-types.html#using-type-predicates

I agree with trying to reduce as many as x as possible, I have tried to resolve them on a per case basis in PR reviews since it makes a huge difference in readability before/ after.

[holgerd77]

The idea is really cool yeah! 😄 It doesn't apply on this context though I would very much say. The supportsEIP() (or however we call it) method is explicitly not about the tx type but about some certain capability. So for 2930 (access lists) it is just not decidable on what to cast (lowest common denominator AccessListEIP2930Transaction or FeeMarketEIP1559Transaction). I would assume it is rather not good to make assumptions here (e.g.: let's cast to the lowest common denominator) since this might not be appropriate for the specific usage context. And at the end there is no such thing as "transaction hierarchy", tx types are at the end completely independent entities, so it might even be wrong to talk about "lowest common denominator" in this context at all.

And with a potential EIP-155 addition this concept breaks completely.

Alternative ideas:

Might it be possible to either:

a) add this to the type getter instead, this then gets deterministic on a call of this method and this would also be more consistent from a usage context perspective I guess (if someone wants to assume a 1559 tx in e.g. an if clause one can also use a tx.type === 2 switch)
b) if possible likely optimal: add this typeguard after the constructor is called (no idea if this is technically possible, also have no insights on typeguards)

An additional thought: could we also refactor the _signedTxImplementsEIP155 (packages\tx\src\legacyTransaction.ts) boolean to use that supportsEIP method also?

Yes, I will add this along the rework, makes super-much sense, txs working in a EIP-155 context is a super-important tx capability and we are dealing with this in a far too implicit manner for my taste right now. I guess we need to go the way here to deprecate _signedTxImplementsEIP155() methods - even if considered private - since there is some higher chance that people are calling this specific one for various custom reasons.

[gabrocheleau]

An additional thought: could we also refactor the _signedTxImplementsEIP155 (packages\tx\src\legacyTransaction.ts) boolean to use that supportsEIP method also?

[holgerd77]

Thanks for not merging this in yet and continue on the discussion, I also had some additional thoughts on this and think this is better off with some rework (@acolytec3 so far with: merging early is always/mostly good, lol 😛).

I think the semantics on this is really tricky here. I thought a bit more on this confusion Andrew had - and I initially had as well - with the similarity to the Common isActivatedEIP() method and I came to the conclusion here that the similarity of these methods I stated above as being nice ("this aligns relatively well with existing Common syntax on lines like if (opts.tx.supportsEIP(2718) && this._common.isActivatedEIP(2718)) {") on second thought is rather a bug than a feature. The semantics of these two things are actually quite different.

The naming I've chosen here with supportsEIP() is actually problematic, since this doesn't differentiate on "theoretically supports" and "this is currently active". This method is not so much about an EIP "being supported" but about a capability (I also used this term actually when re-describing to Andrew, often some indicator of some actually good wording) being active. So I guess the following would be a lot better wording and API:

tx.active(Capabilities.EIP1559_FEE_MARKET)

I guess that's a lot more accurate and descriptive (I first had tx.activeCapability(Capabilities.EIP1559_FEE_MARKET) here, but I guess we can skip this first Capability here since it's getting redundant with the enum referencing, so there is nothing lost on readability, rather the contrary). I would also redo with the enum idea from @ryanio. On a second thought this is a lot more descriptive and 2930 status just reads a lot better with Capabilities.EIP2930_Access_Lists than having to mentally transform the EIP number to what this actually is (or worse: not knowing what 2930 means at all). And the situation is actually a bit different than with these more low level Common.isActivatedEIP() switches which will likely be used in another - more "known" - context (e.g. for us internally in our VM implementation) than if someone wants to get information about tx features to e.g. display some associated frontend information.

Side note: I will finally try out enums in JavaScript to be on the safe side here.

Long story short (not really possible any more, lol 😜): I am so convinced here that I will directly give this a rework. Will make a local copy of my branch though in case anyone has strong arguments to revert.

[holgerd77]

Ok, I finally made the enum test for JavaScript (not sure if this ever was an issue for you, but for me it was).

So I did the following TypeScript script:

export enum Hello {
    World = "world",
    Everyone = "everyone",
}

export function hello(to: Hello) {
    return `Hello ${to}`
}

This can be used from another TypeScript file like this:

import { Hello } from './hello'
import { hello } from './hello'
hello(Hello.World) // 'Hello world'

Then I have compiled the first "library" (lol) file with tsc hello.ts and then tried with JavaScript:

const Hello = require('./hello').Hello
const hello = require('./hello').hello
hello(Hello.World) // 'Hello world'

This works pretty well, call syntax is the same, so I guess we are fine to use here (and for other use cases). 😄

[holgerd77]

Update: argh, sorry for this back-and-forth, but when seeing this transaction signature usage a bit tx.active(Capabilities.EIP2718TypedTransaction) I am thinking this is also a bit misleading since this indicates a bit that a capability for the same tx like the tx type could also be inactive, which also makes not much sense. So I am considering switching back to what Ryan initially suggested and do tx.supports() together with the enums.

I was first thinking "hmm, and then again the tx.active() would rather fit better for the EIP-155 use case since EIP-155 can be somewhat active or not". But when rethinking: EIP-155 is also more like an implicit tx type than a feature which can be turned on and off (and therefore be "active" or not) since a switch can't happen over the lifetime of a tx. From this PoV tx.supports() would fit here as well.

[holgerd77]

Ok, also added EIP-155 as a capability in 505c856, this comes along with some substantial reworking of the code. Generally I am pretty happy with this, nevertheless would need a somewhat thorough review.

One test was failing on this and I had to add a somewhat ugly hack in 1c29006 to get this fixed. This is for a very questionable special case, the test is legacy.spec.ts -> sign(), verifySignature(): should ignore any previous signature when decided if EIP155 should be used in a new one and the case is that we have got a legacy tx after spuriousDragon with a non-EIP155 conforming signature and want to recreate a signature (where EIP155 should be applied).

Phew. On first thought I wonder if tx.sign() should not just throw in case this is called on a signed tx, I would very much assume that there had been some discussion around this in the past which I likely missed.

Would be interested to hear a short statement from @alcuadrado on this, Patricio, wonder if this test actually also has been added by you?

In doubt I guess we could live with this hack though. The overall EIP-155 related signing code is a lot cheaper now in return and the hack is very much isolated.

Ok. Ready for review. 😄

[emersonmacro]

A lot of the details here are new to me so I would certainly defer to @ryanio and others for official approval, but the tx.supports() and Capabilities enum pattern makes sense to me, and none of the code changes jumped out to me as problematic. 👍

[ryanio]

looks really great, I didn't have time to do a full review tonight but will do in the morning!

[ryanio]

looks awesome, love how this PR ended up and really enjoying the improved code and readability.