-
Notifications
You must be signed in to change notification settings - Fork 772
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
tx: improve tx capability handling #3010
Changes from 10 commits
2992719
7373569
76160e3
cf53ddd
eaf893d
0f708ea
23dcf35
ff58d43
8411952
77632bb
8b08c11
32c7de8
e410654
903545d
3c576cd
189baf8
22b7366
ecd4a13
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
import { RLP } from '@ethereumjs/rlp' | ||
import { concatBytes } from '@ethereumjs/util' | ||
import { keccak256 } from 'ethereum-cryptography/keccak.js' | ||
|
||
import { txTypeBytes } from '../util.js' | ||
|
||
import { errorMsg } from './legacy.js' | ||
|
||
import type { EIP2718CompatibleTxInterface, TypedTransaction } from '../types' | ||
import type { Input } from '@ethereumjs/rlp' | ||
|
||
export function getHashedMessageToSign(tx: EIP2718CompatibleTxInterface): Uint8Array { | ||
return keccak256(tx.getMessageToSign()) | ||
} | ||
|
||
export function serialize(tx: EIP2718CompatibleTxInterface, base?: Input): Uint8Array { | ||
return concatBytes(txTypeBytes(tx.type), RLP.encode(base ?? tx.raw())) | ||
} | ||
|
||
export function validateYParity(tx: TypedTransaction) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it not possible to type with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Forgot to adjust after refactoring, fixed. |
||
const { v } = tx | ||
if (v !== undefined && v !== BigInt(0) && v !== BigInt(1)) { | ||
const msg = errorMsg(tx, 'The y-parity of the transaction should either be 0 or 1') | ||
throw new Error(msg) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,42 +1,12 @@ | ||
import { RLP } from '@ethereumjs/rlp' | ||
import { concatBytes, hexToBytes } from '@ethereumjs/util' | ||
import { keccak256 } from 'ethereum-cryptography/keccak.js' | ||
|
||
import { BaseTransaction } from '../baseTransaction.js' | ||
import { type TypedTransaction } from '../types.js' | ||
import { AccessLists } from '../util.js' | ||
|
||
import type { Transaction, TransactionType } from '../types.js' | ||
import * as Legacy from './legacy.js' | ||
|
||
type TypedTransactionEIP2930 = Exclude<TypedTransaction, Transaction[TransactionType.Legacy]> | ||
import type { EIP2930CompatibleTxInterface } from '../types.js' | ||
|
||
/** | ||
* The amount of gas paid for the data in this tx | ||
*/ | ||
export function getDataFee(tx: TypedTransactionEIP2930): bigint { | ||
if (tx['cache'].dataFee && tx['cache'].dataFee.hardfork === tx.common.hardfork()) { | ||
return tx['cache'].dataFee.value | ||
} | ||
|
||
let cost = BaseTransaction.prototype.getDataFee.bind(tx)() | ||
cost += BigInt(AccessLists.getDataFeeEIP2930(tx.accessList, tx.common)) | ||
|
||
if (Object.isFrozen(tx)) { | ||
tx['cache'].dataFee = { | ||
value: cost, | ||
hardfork: tx.common.hardfork(), | ||
} | ||
} | ||
|
||
return cost | ||
} | ||
|
||
export function getHashedMessageToSign(tx: TypedTransactionEIP2930): Uint8Array { | ||
return keccak256(tx.getMessageToSign()) | ||
} | ||
|
||
export function serialize(tx: TypedTransactionEIP2930): Uint8Array { | ||
const base = tx.raw() | ||
const txTypeBytes = hexToBytes('0x' + tx.type.toString(16).padStart(2, '0')) | ||
return concatBytes(txTypeBytes, RLP.encode(base)) | ||
export function getDataFee(tx: EIP2930CompatibleTxInterface): bigint { | ||
return Legacy.getDataFee(tx, BigInt(AccessLists.getDataFeeEIP2930(tx.accessList, tx.common))) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,25 +5,25 @@ import { | |
bigIntToUnpaddedBytes, | ||
bytesToBigInt, | ||
bytesToHex, | ||
concatBytes, | ||
equalsBytes, | ||
hexToBytes, | ||
toBytes, | ||
validateNoLeadingZeroes, | ||
} from '@ethereumjs/util' | ||
|
||
import { BaseTransaction } from './baseTransaction.js' | ||
import * as EIP1559 from './capabilities/eip1559.js' | ||
import * as EIP2718 from './capabilities/eip2718.js' | ||
import * as EIP2930 from './capabilities/eip2930.js' | ||
import * as Generic from './capabilities/generic.js' | ||
import * as Legacy from './capabilities/legacy.js' | ||
import { TransactionType } from './types.js' | ||
import { AccessLists } from './util.js' | ||
import { AccessLists, txTypeBytes } from './util.js' | ||
|
||
import type { | ||
AccessList, | ||
AccessListBytes, | ||
TxData as AllTypesTxData, | ||
TxValuesArray as AllTypesTxValuesArray, | ||
EIP1559CompatibleTxInterface, | ||
JsonTx, | ||
TxOptions, | ||
} from './types.js' | ||
|
@@ -32,17 +32,16 @@ import type { Common } from '@ethereumjs/common' | |
type TxData = AllTypesTxData[TransactionType.FeeMarketEIP1559] | ||
type TxValuesArray = AllTypesTxValuesArray[TransactionType.FeeMarketEIP1559] | ||
|
||
const TRANSACTION_TYPE_BYTES = hexToBytes( | ||
'0x' + TransactionType.FeeMarketEIP1559.toString(16).padStart(2, '0') | ||
) | ||
|
||
/** | ||
* Typed transaction with a new gas fee market mechanism | ||
* | ||
* - TransactionType: 2 | ||
* - EIP: [EIP-1559](https://eips.ethereum.org/EIPS/eip-1559) | ||
*/ | ||
export class FeeMarketEIP1559Transaction extends BaseTransaction<TransactionType.FeeMarketEIP1559> { | ||
export class FeeMarketEIP1559Transaction | ||
extends BaseTransaction<TransactionType.FeeMarketEIP1559> | ||
implements EIP1559CompatibleTxInterface<TransactionType.FeeMarketEIP1559> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is not structurally correct to add this 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
{ | ||
public readonly chainId: bigint | ||
public readonly accessList: AccessListBytes | ||
public readonly AccessListJSON: AccessList | ||
|
@@ -72,7 +71,10 @@ export class FeeMarketEIP1559Transaction extends BaseTransaction<TransactionType | |
* accessList, signatureYParity, signatureR, signatureS])` | ||
*/ | ||
public static fromSerializedTx(serialized: Uint8Array, opts: TxOptions = {}) { | ||
if (equalsBytes(serialized.subarray(0, 1), TRANSACTION_TYPE_BYTES) === false) { | ||
if ( | ||
equalsBytes(serialized.subarray(0, 1), txTypeBytes(TransactionType.FeeMarketEIP1559)) === | ||
false | ||
) { | ||
throw new Error( | ||
`Invalid serialized tx input: not an EIP-1559 transaction (wrong tx type, expected: ${ | ||
TransactionType.FeeMarketEIP1559 | ||
|
@@ -189,8 +191,8 @@ export class FeeMarketEIP1559Transaction extends BaseTransaction<TransactionType | |
throw new Error(msg) | ||
} | ||
|
||
Generic.validateYParity(this) | ||
Generic.validateHighS(this) | ||
EIP2718.validateYParity(this) | ||
Legacy.validateHighS(this) | ||
|
||
const freeze = opts?.freeze ?? true | ||
if (freeze) { | ||
|
@@ -254,7 +256,7 @@ export class FeeMarketEIP1559Transaction extends BaseTransaction<TransactionType | |
* the RLP encoding of the values. | ||
*/ | ||
serialize(): Uint8Array { | ||
return EIP2930.serialize(this) | ||
return EIP2718.serialize(this) | ||
} | ||
|
||
/** | ||
|
@@ -269,9 +271,7 @@ export class FeeMarketEIP1559Transaction extends BaseTransaction<TransactionType | |
* ``` | ||
*/ | ||
getMessageToSign(): Uint8Array { | ||
const base = this.raw().slice(0, 9) | ||
const message = concatBytes(TRANSACTION_TYPE_BYTES, RLP.encode(base)) | ||
return message | ||
return EIP2718.serialize(this, this.raw().slice(0, 9)) | ||
} | ||
|
||
/** | ||
|
@@ -282,7 +282,7 @@ export class FeeMarketEIP1559Transaction extends BaseTransaction<TransactionType | |
* serialized and doesn't need to be RLP encoded any more. | ||
*/ | ||
getHashedMessageToSign(): Uint8Array { | ||
return EIP2930.getHashedMessageToSign(this) | ||
return EIP2718.getHashedMessageToSign(this) | ||
} | ||
|
||
/** | ||
|
@@ -292,7 +292,7 @@ export class FeeMarketEIP1559Transaction extends BaseTransaction<TransactionType | |
* Use {@link FeeMarketEIP1559Transaction.getMessageToSign} to get a tx hash for the purpose of signing. | ||
*/ | ||
public hash(): Uint8Array { | ||
return Generic.hash(this) | ||
return Legacy.hash(this) | ||
} | ||
|
||
/** | ||
|
@@ -306,7 +306,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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! 🥳 |
||
} | ||
|
||
protected _processSignature(v: bigint, r: Uint8Array, s: Uint8Array) { | ||
|
@@ -363,6 +363,6 @@ export class FeeMarketEIP1559Transaction extends BaseTransaction<TransactionType | |
* @hidden | ||
*/ | ||
protected _errorMsg(msg: string) { | ||
return Generic.errorMsg(this, msg) | ||
return Legacy.errorMsg(this, msg) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 withInterface
orI
or the like in the monorepo.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shortened by removing the unnecessary Interface suffix.