tx: improve tx capability handling #3010

gabrocheleau · 2023-09-05T02:29:12Z

This PR addresses comments and suggestions for improvements that were made on #2993

codecov · 2023-09-05T02:32:03Z

Codecov Report

Merging #3010 (ecd4a13) into master (04aa4da) will decrease coverage by 0.02%.
The diff coverage is 98.44%.

Additional details and impacted files

Flag	Coverage Δ
block	`88.73% <ø> (ø)`
blockchain	`92.58% <ø> (ø)`
client	`87.47% <ø> (ø)`
common	`98.18% <ø> (ø)`
ethash	`∅ <ø> (∅)`
evm	`71.70% <ø> (ø)`
statemanager	`89.91% <ø> (ø)`
trie	`90.04% <ø> (-0.15%)`	⬇️
tx	`96.35% <98.44%> (-0.04%)`	⬇️
util	`86.78% <ø> (ø)`
vm	`80.42% <ø> (ø)`

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

holgerd77

Thanks a lot, looks already pretty great respectively forms to a great picture and gets us a lot closer to a modular architecture! 🤩

Some structural comments to address.

holgerd77 · 2023-09-08T14:03:29Z

packages/tx/src/types.ts

+  extends TransactionInterface<T> {
+  readonly chainId: bigint
+  readonly accessList: AccessListBytes
+  readonly AccessListJSON: AccessList


Sorry to be a bit pedantic here, but access lists have really nothing to do with the generic introduction of typed transactions - and so with EIP2718 - and if we have this here, we still have something structurally wrong (e.g. using this type from here while we should have used the EIP2930Compatible type).

Generally this looks great though with these new interfaces! 🤩

That's true, EIP2930 capabilities should be left within their own scope, will adjust.

holgerd77 · 2023-09-08T14:06:33Z

packages/tx/src/capabilities/eip2718.ts

+  return keccak256(tx.getMessageToSign())
+}
+
+export function serialize(tx: EIP2718CompatibleTxInterface, base?: Input): Uint8Array {


Maybe we drop the Interface from these names to get it a bit shorter? 🤔 No super strong preference, but we also mostly (exception: these very important reused interfaces in Common) do not explicitly name interfaces with Interface or Ior the like in the monorepo.

(but the shortening would be the greater argument here, especially if there are more arguments to follow, then this likely often moves over unnecessarily into two lines)

Shortened by removing the unnecessary Interface suffix.

holgerd77 · 2023-09-08T14:07:44Z

packages/tx/src/capabilities/eip2718.ts

+  return concatBytes(txTypeBytes(tx.type), RLP.encode(base ?? tx.raw()))
+}
+
+export function validateYParity(tx: TypedTransaction) {


Is it not possible to type with EIP2718CompatibleTxInterface here?

Forgot to adjust after refactoring, fixed.

holgerd77 · 2023-09-08T14:19:36Z

packages/tx/src/types.ts

@@ -152,6 +171,32 @@ export interface TransactionInterface<T extends TransactionType> {
  errorStr(): string
 }


What is somewhat missing here which would complement the picture would be a LegacyTxInterface.

I know this is likely the hardest one with eventual conflicts on re-/cross-used functionality or property access.

But did you try?

That would be great if we would/could achieve that. Since here we - again - at least to some extend replicate this old hierarchy structure which we would optimally want to completely leave behind.

Good point. Added a LegacyTxInterface, makes this more consistent and complete.

holgerd77 · 2023-09-08T14:23:39Z

packages/tx/src/eip1559Transaction.ts

-export class FeeMarketEIP1559Transaction extends BaseTransaction<TransactionType.FeeMarketEIP1559> {
+export class FeeMarketEIP1559Transaction
+  extends BaseTransaction<TransactionType.FeeMarketEIP1559>
+  implements EIP1559CompatibleTxInterface<TransactionType.FeeMarketEIP1559>


I think this is not structurally correct to add this implements note respectively is both redundant and not complete at the same time. So I think we should remove.

If we would want to do this correctly/completely we would need to implement from several compatibility interfaces (here e.g. also from the EIP2930 interface), but multiple inheritance unfortunately doesn't exist in JavaScript (I thought a bit along these lines too some 1-2 years ago when thinking about how we could refactor this stuff).

Yeah, I actually went back and forth on this. I agree about the point concerning multiple inheritance, but since all of the interfaces are nested within each other (eip1559 inherits from eip2930, and eip4844 inherits from eip1559), I figured this wasn't a problem. However I agree that this would not be "correct", especially if we want to treat capabilities as independent from one another.

There are no type errors even if we remove this ìmplements (typescript automatically figures out whether or not the class fits the necessary interface when we use the Capabilities, regardless of if we explicitly mention that it implements said interface. I'll go ahead and remove this.

holgerd77 · 2023-09-08T14:25:03Z

packages/tx/src/eip1559Transaction.ts

@@ -306,7 +311,7 @@ export class FeeMarketEIP1559Transaction extends BaseTransaction<TransactionType
   * Returns the public key of the sender
   */
  public getSenderPublicKey(): Uint8Array {
-    return Generic.getSenderPublicKey(this)
+    return Legacy.getSenderPublicKey(this)


Yeah, but generally this reads super-great, to always already have the heritage of the feature read out of these calls, so that one can associate how the tx type is composed and where the functionality comes from! 🥳

holgerd77 · 2023-09-08T14:26:05Z

packages/tx/src/eip4844Transaction.ts

-export class BlobEIP4844Transaction extends BaseTransaction<TransactionType.BlobEIP4844> {
+export class BlobEIP4844Transaction
+  extends BaseTransaction<TransactionType.BlobEIP4844>
+  implements EIP4844CompatibleTxInterface<TransactionType.BlobEIP4844>


Here again (and for the other tx types too), I think we should remove.

This blurs again the capability concept.

…face

gabrocheleau · 2023-09-11T00:38:09Z

Everything should be addressed @holgerd77, can be re-reviewed! :)

holgerd77

Thanks a lot, this looks really fantastic 😍, I didn't imagine this to turn out SO structurally clean on all fronts, totally love this! 🤩

Will merge.

tx: TxCapability interfaces and rename generic capability to legacy

2992719

gabrocheleau added PR state: needs review type: enhancement package: tx labels Sep 5, 2023

gabrocheleau added 5 commits September 5, 2023 07:06

Merge branch 'master' into tx/consolidation-improvements

7373569

tx: txTypeBytes helper

76160e3

tx: add missing methods and props to tx interfaces

cf53ddd

tx: refactor and improve serialize helper

eaf893d

tx: implement update serialize helper and txTypeBytes helper

0f708ea

gabrocheleau mentioned this pull request Sep 5, 2023

tx: consolidate generic tx capabilities #2993

Merged

gabrocheleau added 4 commits September 5, 2023 09:13

Merge branch 'master' into tx/consolidation-improvements

23dcf35

Merge branch 'master' into tx/consolidation-improvements

ff58d43

tx: refactor getDataFee for use in legacyTransaction

8411952

tx: remove redundant prop from EIP4844CompatibleTxInterface

77632bb

holgerd77 requested changes Sep 8, 2023

View reviewed changes

gabrocheleau added 5 commits September 10, 2023 20:22

tx: shorten compatibletxInterface name

8b08c11

tx: typedTransaction -> EIP2718CompatibleTx

32c7de8

tx: remove implements interface

e410654

tx: add legacytxinterface and move accesslists props to eip2930 inter…

903545d

…face

Merge branch 'master' into tx/consolidation-improvements

3c576cd

gabrocheleau requested a review from holgerd77 September 11, 2023 00:38

gabrocheleau added 3 commits September 14, 2023 07:52

Merge branch 'master' into tx/consolidation-improvements

189baf8

Merge branch 'master' into tx/consolidation-improvements

22b7366

Merge branch 'master' into tx/consolidation-improvements

ecd4a13

holgerd77 approved these changes Sep 14, 2023

View reviewed changes

holgerd77 merged commit 67a20de into master Sep 14, 2023
42 checks passed

holgerd77 deleted the tx/consolidation-improvements branch September 14, 2023 19:45

jochem-brouwer mentioned this pull request Oct 9, 2024

TX refactor PR #3733

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tx: improve tx capability handling #3010

tx: improve tx capability handling #3010

gabrocheleau commented Sep 5, 2023 •

edited

Loading

codecov bot commented Sep 5, 2023 •

edited

Loading

holgerd77 left a comment

holgerd77 Sep 8, 2023

gabrocheleau Sep 11, 2023

holgerd77 Sep 8, 2023

holgerd77 Sep 8, 2023

gabrocheleau Sep 11, 2023

holgerd77 Sep 8, 2023

gabrocheleau Sep 11, 2023

holgerd77 Sep 8, 2023

gabrocheleau Sep 11, 2023

holgerd77 Sep 8, 2023

gabrocheleau Sep 11, 2023

holgerd77 Sep 8, 2023

holgerd77 Sep 8, 2023

gabrocheleau Sep 11, 2023 •

edited

Loading

gabrocheleau commented Sep 11, 2023

holgerd77 left a comment

		@@ -152,6 +171,32 @@ export interface TransactionInterface<T extends TransactionType> {
		errorStr(): string
		}

tx: improve tx capability handling #3010

tx: improve tx capability handling #3010

Conversation

gabrocheleau commented Sep 5, 2023 • edited Loading

codecov bot commented Sep 5, 2023 • edited Loading

Codecov Report

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrocheleau Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

gabrocheleau commented Sep 11, 2023

holgerd77 left a comment

Choose a reason for hiding this comment

gabrocheleau commented Sep 5, 2023 •

edited

Loading

codecov bot commented Sep 5, 2023 •

edited

Loading

gabrocheleau Sep 11, 2023 •

edited

Loading