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

tx: add getDataFee to tx cache #1532

Merged
merged 10 commits into from
Oct 27, 2021
3 changes: 3 additions & 0 deletions packages/tx/src/baseTransaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {

interface TransactionCache {
hash: Buffer | undefined
dataFee: BN | undefined
}

/**
Expand All @@ -49,6 +50,7 @@ export abstract class BaseTransaction<TransactionObject> {

protected cache: TransactionCache = {
hash: undefined,
dataFee: undefined,
}

/**
Expand Down Expand Up @@ -185,6 +187,7 @@ export abstract class BaseTransaction<TransactionObject> {
for (let i = 0; i < this.data.length; i++) {
this.data[i] === 0 ? (cost += txDataZero) : (cost += txDataNonZero)
}

return new BN(cost)
}

Expand Down
9 changes: 9 additions & 0 deletions packages/tx/src/eip1559Transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -236,8 +236,17 @@ export default class FeeMarketEIP1559Transaction extends BaseTransaction<FeeMark
* The amount of gas paid for the data in this tx
*/
getDataFee(): BN {
if (Object.isFrozen(this) && this.cache.dataFee) {
return this.cache.dataFee
}

const cost = super.getDataFee()
cost.iaddn(AccessLists.getDataFeeEIP2930(this.accessList, this.common))

if (Object.isFrozen(this) && !this.cache.dataFee) {
this.cache.dataFee = cost
}
Copy link
Member

Choose a reason for hiding this comment

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

On the first if condition you can remove the Object.isFrozen(this) check and on the second the this.cache.dataFee check. Guess this reads a bit better and is less redundant, also for the other tx types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call - updated and it does read better


return cost
}

Expand Down
9 changes: 9 additions & 0 deletions packages/tx/src/eip2930Transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,17 @@ export default class AccessListEIP2930Transaction extends BaseTransaction<Access
* The amount of gas paid for the data in this tx
*/
getDataFee(): BN {
if (Object.isFrozen(this) && this.cache.dataFee) {
return this.cache.dataFee
}

const cost = super.getDataFee()
cost.iaddn(AccessLists.getDataFeeEIP2930(this.accessList, this.common))

if (Object.isFrozen(this) && !this.cache.dataFee) {
this.cache.dataFee = cost
}

return cost
}

Expand Down
13 changes: 13 additions & 0 deletions packages/tx/src/legacyTransaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,19 @@ export default class Transaction extends BaseTransaction<Transaction> {
}
}

/**
* The amount of gas paid for the data in this tx
*/
getDataFee(): BN {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to add this? would it not work falling back to the default baseTransaction method?

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's a little weird because the cache is created in baseTransaction, but caching doesn't actually happen in baseTransaction because it doesn't have the option to be frozen. The freeze option only exists on the derived classes. That's my reading of the code but I might be misinterpreting it

Copy link
Contributor

Choose a reason for hiding this comment

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

hm, I think Object.isFrozen(this) would still work in the baseTransaction, but would have to try it out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, just remembered, my first attempt was to implement the caching in baseTransaction, but the issue I ran into was that eip1559Transaction and eip2930Transaction both call super.getDataFee() but then modify the value in their own implementations of getDataFee. But the value of getDataFee was getting calculated and cached at the baseTransaction level, so the incorrect value was being returned in the derived classes.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, guess that makes sense.

Can you structurally align this method with the other two methods? So with this cost constant and generally the same order of calling? That would be easier to read and compare.

if (Object.isFrozen(this)) {
if (!this.cache.dataFee) {
this.cache.dataFee = super.getDataFee()
}
return this.cache.dataFee
}
return super.getDataFee()
}

/**
* The up front amount that an account must have for this transaction to be valid
*/
Expand Down
10 changes: 10 additions & 0 deletions packages/tx/test/eip1559.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -201,4 +201,14 @@ tape('[FeeMarketEIP1559Transaction]', function (t) {
}, 'total fee must be the larger of the two')
st.end()
})

t.test('getDataFee()', function (st) {
let tx = FeeMarketEIP1559Transaction.fromTxData({}, { common })
st.ok(tx.getDataFee().toNumber() === 0, 'Should return data fee when frozen')

tx = FeeMarketEIP1559Transaction.fromTxData({}, { common, freeze: false })
st.ok(tx.getDataFee().toNumber() === 0, 'Should return data fee when not frozen')

st.end()
})
})
3 changes: 3 additions & 0 deletions packages/tx/test/legacy.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ tape('[Transaction]', function (t) {
tx = Transaction.fromValuesArray(txFixtures[3].raw.map(toBuffer))
st.equals(tx.getDataFee().toNumber(), 1716)

tx = Transaction.fromValuesArray(txFixtures[3].raw.map(toBuffer), { freeze: false })
st.equals(tx.getDataFee().toNumber(), 1716)

st.end()
})

Expand Down
10 changes: 10 additions & 0 deletions packages/tx/test/typedTxsAndEIP2930.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -528,4 +528,14 @@ tape('[AccessListEIP2930Transaction] -> Class Specific Tests', function (t) {

t.end()
})

t.test('getDataFee()', function (st) {
let tx = AccessListEIP2930Transaction.fromTxData({}, { common })
st.ok(tx.getDataFee().toNumber() === 0, 'Should return data fee when frozen')

tx = AccessListEIP2930Transaction.fromTxData({}, { common, freeze: false })
st.ok(tx.getDataFee().toNumber() === 0, 'Should return data fee when not frozen')

st.end()
})
})