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

EIP1559 #4155

Merged
merged 42 commits into from
Jul 21, 2021
Merged

EIP1559 #4155

Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
ec47e41
npm run build for 1.4.0-rc.0
spacesailor24 Jun 18, 2021
d50f17a
v1.4.0-rc.0
spacesailor24 Jun 18, 2021
87738b7
Update scripts/e2e.geth.instamine.sh to use v1.10.3 of Geth
spacesailor24 Jun 24, 2021
059be6d
Update scripts/e2e.geth.instamine.sh to use v1.10.3 of Geth
spacesailor24 Jun 24, 2021
7b2acdf
Fix typo
spacesailor24 Jun 24, 2021
bc8a427
Fix merge conflict
spacesailor24 Jun 24, 2021
5c11080
WIP
spacesailor24 Jun 25, 2021
c53a87e
Merge branch '1.x' into wyatt/eip1559
spacesailor24 Jun 25, 2021
f963659
Fix type in error catch
spacesailor24 Jun 28, 2021
3e4fcbe
Remove commented code
spacesailor24 Jun 30, 2021
7681019
Update index for tx pricing info
spacesailor24 Jun 30, 2021
3212c05
Correct order of if statements to properly assign tx type
spacesailor24 Jun 30, 2021
43a755f
Update maxFeePerGas calculation
spacesailor24 Jun 30, 2021
51f030e
Init _handleTxType function
spacesailor24 Jun 30, 2021
3b3efb2
Update _handleTxPricing to use gasPrice for eip-1559 txs
spacesailor24 Jun 30, 2021
b458b63
Fix bugs in _handleTxPricing
spacesailor24 Jun 30, 2021
1b66314
Update tests for eip-1559 changes
spacesailor24 Jun 30, 2021
ad5a22e
Fix BN bug: .mul only accepts BN instances
spacesailor24 Jun 30, 2021
4ebd78a
eth.accounts.signTransaction: Add London tests
spacesailor24 Jun 30, 2021
cd490a8
Fix merge conflicts
spacesailor24 Jul 3, 2021
43fc7aa
Add EIP-1559 test without AccessList
spacesailor24 Jul 3, 2021
5c5a517
Add tx.common.hardfork check to _handleTxType
spacesailor24 Jul 3, 2021
34e5cf6
Bug fixes
spacesailor24 Jul 3, 2021
72c9898
Add additional undefined check in _handleTxType
spacesailor24 Jul 3, 2021
073be03
Add additional check for tx.hardfork in _handleTxType
spacesailor24 Jul 3, 2021
1e3d662
Add additional check for tx.hardfork in _handleTxType
spacesailor24 Jul 3, 2021
2bb8c63
Update CHANGELOG
spacesailor24 Jul 3, 2021
572e416
Handling EIP1559 transactions in outputTransactionFormatter (#4167)
corymsmith Jul 7, 2021
abbc390
Revert geth docker version tag
spacesailor24 Jul 8, 2021
ef39538
Add additional EIP-2930 and EIP-1559 tests
spacesailor24 Jul 8, 2021
494f260
Merge branch '1.x' into wyatt/eip1559
spacesailor24 Jul 10, 2021
2437073
Merge branch 'wyatt/eip1559' of github.com:ChainSafe/web3.js into wya…
spacesailor24 Jul 10, 2021
b3ab709
EIP 1559 Debug #2 (#4171)
spacesailor24 Jul 19, 2021
dc810b3
Mergin conflicts
spacesailor24 Jul 19, 2021
e7d3833
Update error message
spacesailor24 Jul 20, 2021
d9c509a
Update error message
spacesailor24 Jul 20, 2021
2a90980
Merge branch 'wyatt/eip1559' of github.com:ChainSafe/web3.js into wya…
spacesailor24 Jul 20, 2021
7324eec
Update use of tx.type
spacesailor24 Jul 21, 2021
d4f1363
Replace hardfork strings with enum
spacesailor24 Jul 21, 2021
f2ca0d8
Type check refactors for _handleTxPricing
spacesailor24 Jul 21, 2021
e0e19f7
Resolve tx.gasPrice if set and tx.type < 0x2 in _handleTxPricing
spacesailor24 Jul 21, 2021
c5be542
Fix _handleTxType logic
spacesailor24 Jul 21, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -400,4 +400,10 @@ Released with 1.0.0-beta.37 code base.

- Removing the underscore package

## [Unreleased]
## [Unreleased]

## [1.5.0]

### Added

- London transaction support (#4155)
14,889 changes: 5,598 additions & 9,291 deletions package-lock.json

Large diffs are not rendered by default.

9 changes: 8 additions & 1 deletion packages/web3-core-helpers/src/formatters.js
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,14 @@ var outputTransactionFormatter = function (tx) {
tx.transactionIndex = utils.hexToNumber(tx.transactionIndex);
tx.nonce = utils.hexToNumber(tx.nonce);
tx.gas = utils.hexToNumber(tx.gas);
tx.gasPrice = outputBigNumberFormatter(tx.gasPrice);
if (tx.gasPrice)
tx.gasPrice = outputBigNumberFormatter(tx.gasPrice);
if (tx.maxFeePerGas)
tx.maxFeePerGas = outputBigNumberFormatter(tx.maxFeePerGas);
if (tx.maxPriorityFeePerGas)
tx.maxPriorityFeePerGas = outputBigNumberFormatter(tx.maxPriorityFeePerGas);
if (tx.type)
tx.type = utils.hexToNumber(tx.type);
tx.value = outputBigNumberFormatter(tx.value);

if (tx.to && utils.isAddress(tx.to)) { // tx.to could be `0x0` or `null` while contract creation
Expand Down
136 changes: 124 additions & 12 deletions packages/web3-eth-accounts/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,17 @@ var Accounts = function Accounts() {
}, function() {
return 'latest';
}]
})
}),
new Method({
name: 'getBlockByNumber',
call: 'eth_getBlockByNumber',
params: 2,
inputFormatter: [function(blockNumber) {
return blockNumber ? utils.toHex(blockNumber) : 'latest'
}, function() {
return false
}]
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a new method?

Copy link
Contributor Author

@spacesailor24 spacesailor24 Jun 30, 2021

Choose a reason for hiding this comment

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

Sorta, this package doesn't make use of the RPC wrappers found in web3-eth, instead needed methods are redefined in the package. I added this method to be able to get a block to find the baseFeePerGas

Choose a reason for hiding this comment

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

You mean we can't leverage the getBlock (also refs blockCall) because it's defined in the web3-eth package and not accessible from web3-eth-accounts?
In this case don't we have a dependency or at least a DRY issue that could be addressed somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AndreMiras Yes, and we plan to address the problematic architecture in 4.x (our rewrite), so for now it's just about making the least amount of change to 1.x as possible, to refrain from introducing additional bugs

];
// attach methods to this._ethereumCall
this._ethereumCall = {};
Expand Down Expand Up @@ -157,11 +167,7 @@ Accounts.prototype.signTransaction = function signTransaction(tx, privateKey, ca
transaction.data = transaction.data || '0x';
transaction.value = transaction.value || '0x';
transaction.gasLimit = transaction.gasLimit || transaction.gas;
transaction.type = "0x0"; // default to legacy
if (transaction.accessList) {
// EIP-2930
transaction.type = "0x01"
}
if (transaction.type === '0x1' && transaction.accessList === undefined) transaction.accessList = []

// Because tx has no @ethereumjs/tx signing options we use fetched vals.
if (!hasTxSigningOptions) {
Expand All @@ -172,7 +178,7 @@ Accounts.prototype.signTransaction = function signTransaction(tx, privateKey, ca
networkId: transaction.networkId,
chainId: transaction.chainId
},
transaction.hardfork || "berlin"
transaction.hardfork || "london"
);

delete transaction.networkId;
Expand All @@ -185,7 +191,7 @@ Accounts.prototype.signTransaction = function signTransaction(tx, privateKey, ca
networkId: transaction.common.customChain.networkId,
chainId: transaction.common.customChain.chainId
},
transaction.common.hardfork || "berlin",
transaction.common.hardfork || "london",
);

delete transaction.common;
Expand Down Expand Up @@ -238,23 +244,42 @@ Accounts.prototype.signTransaction = function signTransaction(tx, privateKey, ca
}
}

tx.type = _handleTxType(tx);

// Resolve immediately if nonce, chainId, price and signing options are provided
if (tx.nonce !== undefined && tx.chainId !== undefined && tx.gasPrice !== undefined && hasTxSigningOptions) {
if (
tx.nonce !== undefined &&
tx.chainId !== undefined &&
(
tx.gasPrice !== undefined ||
(
tx.maxFeePerGas !== undefined &&
tx.maxPriorityFeePerGas !== undefined
)
) &&
hasTxSigningOptions
) {
return Promise.resolve(signed(tx));
}

// Otherwise, get the missing info from the Ethereum Node
return Promise.all([
isNot(tx.chainId) ? _this._ethereumCall.getChainId() : tx.chainId,
isNot(tx.gasPrice) ? _this._ethereumCall.getGasPrice() : tx.gasPrice,
isNot(tx.nonce) ? _this._ethereumCall.getTransactionCount(_this.privateKeyToAccount(privateKey).address) : tx.nonce,
isNot(hasTxSigningOptions) ? _this._ethereumCall.getNetworkId() : 1
isNot(hasTxSigningOptions) ? _this._ethereumCall.getNetworkId() : 1,
_handleTxPricing(_this, tx)
]).then(function(args) {
if (isNot(args[0]) || isNot(args[1]) || isNot(args[2]) || isNot(args[3])) {
throw new Error('One of the values "chainId", "networkId", "gasPrice", or "nonce" couldn\'t be fetched: ' + JSON.stringify(args));
}
return signed({ ...tx, chainId: args[0], gasPrice: args[1], nonce: args[2], networkId: args[3]});

return signed({
...tx,
chainId: args[0],
nonce: args[1],
networkId: args[2],
...args[3] // Will either be gasPrice or maxFeePerGas and maxPriorityFeePerGas
});
});
};

Expand Down Expand Up @@ -286,6 +311,93 @@ function _validateTransactionForSigning(tx) {
return;
}

function _handleTxType(tx) {
let txType = tx.type !== undefined ? utils.toHex(tx.type) : '0x0';
// Taken from https://github.com/ethers-io/ethers.js/blob/2a7ce0e72a1e0c9469e10392b0329e75e341cf18/packages/abstract-signer/src.ts/index.ts#L215
const hasEip1559 = (tx.maxFeePerGas !== undefined || tx.maxPriorityFeePerGas !== undefined);
if (tx.gasPrice !== undefined && (tx.type === '0x2' || hasEip1559))
throw Error("eip-1559 transactions don't support gasPrice");
if ((tx.type === '0x1' || tx.type === '0x0') && hasEip1559)
GregTheGreek marked this conversation as resolved.
Show resolved Hide resolved
throw Error("pre-eip-1559 transaction don't support maxFeePerGas/maxPriorityFeePerGas");

if (
hasEip1559 ||
(
(tx.common && tx.common.hardfork && tx.common.hardfork.toLowerCase() === 'london') ||
(tx.hardfork && tx.hardfork.toLowerCase() === 'london')
)
) {
txType = '0x2';
} else if (
tx.accessList ||
(
(tx.common && tx.common.hardfork && tx.common.hardfork.toLowerCase() === 'berlin') ||
GregTheGreek marked this conversation as resolved.
Show resolved Hide resolved
(tx.hardfork && tx.hardfork.toLowerCase() === 'berlin')
)
) {
txType = '0x1';
}

return txType
}

function _handleTxPricing(_this, tx) {
return new Promise((resolve, reject) => {
try {
if (
(
tx.type === undefined ||
tx.type === '0x0' ||
tx.type === '0x1'
GregTheGreek marked this conversation as resolved.
Show resolved Hide resolved
) && tx.gasPrice !== undefined)
GregTheGreek marked this conversation as resolved.
Show resolved Hide resolved
{
// Legacy transaction, return provided gasPrice
resolve({ gasPrice: tx.gasPrice })
} else {
Promise.all([
_this._ethereumCall.getBlockByNumber(),
_this._ethereumCall.getGasPrice()
]).then(responses => {
const [block, gasPrice] = responses;
if (
(tx.type !== '0x0' &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Its already checked https://github.com/ChainSafe/web3.js/pull/4155/files#diff-cf46f2521b1de659430e09e654742a41c3696dbdddc3cd563ca4e2c02d18ffacR352 for legacy Tx types. May be we can remove this double check or is there any use of additional check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So while this is double checking tx.type, it's for different reasons:

Here we are short circuiting and returning gasPrice if it's apart of the tx object and tx.type is 0x0 or 0x1 since these txs will not have the EIP1559 fields and will be using the user provided gasPrice

And here we are verifying we aren't of type 0x0 or 0x1 before we add EIP1559 fields

I admit that this code is a bit convoluted, but it's essentially just a condensed version of Ether's EIP1559 implementation

tx.type !== '0x1') &&
GregTheGreek marked this conversation as resolved.
Show resolved Hide resolved
block && block.baseFeePerGas
) {
// The network supports EIP-1559

// Taken from https://github.com/ethers-io/ethers.js/blob/ba6854bdd5a912fe873d5da494cb5c62c190adde/packages/abstract-provider/src.ts/index.ts#L230
let maxPriorityFeePerGas, maxFeePerGas;

if (tx.gasPrice) {
// Using legacy gasPrice property on an eip-1559 network,
// so use gasPrice as both fee properties
maxPriorityFeePerGas = tx.gasPrice;
maxFeePerGas = tx.gasPrice;
delete tx.gasPrice;
} else {
maxPriorityFeePerGas = tx.maxPriorityFeePerGas || '0x3B9ACA00'; // 1 Gwei
Copy link
Contributor

Choose a reason for hiding this comment

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

Its not part of the EIP ( specs ) to must have default 'priority fee per gas' 1 Gwei. Its only suggested to set this default.

so I suggest It should always be coming from web3 user only, or if we are setting default then must notify / inform user as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was something that was suggested on a call with wallet/library implementers, but we should make a note in the docs

Copy link
Contributor Author

@spacesailor24 spacesailor24 Jul 7, 2021

Choose a reason for hiding this comment

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

We'll add maxPriorityFeePerGas being defaulted to 1 Gwei to the docs, but I think the understanding was that a tx without a priority fee wouldn't get processed/be excluded from the mem pool

maxFeePerGas = tx.maxFeePerGas ||
utils.toHex(
utils.toBN(block.baseFeePerGas)
.mul(utils.toBN(2))
Copy link
Contributor

Choose a reason for hiding this comment

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

Also will be imp to highlight that web3 is doing it and why "baseFeePerGas times 2" to web3 users in doc.

.add(utils.toBN(maxPriorityFeePerGas))
);
}
resolve({ maxFeePerGas, maxPriorityFeePerGas });
} else {
if (tx.maxPriorityFeePerGas || tx.maxFeePerGas)
throw Error("Network doesn't support eip-1559")
resolve({ gasPrice });
}
})
}
} catch (error) {
reject(error)
}
})
}

/* jshint ignore:start */
Accounts.prototype.recoverTransaction = function recoverTransaction(rawTx, txOptions = {}) {
// Rely on EthereumJs/tx to determine the type of transaction
Expand Down
2 changes: 1 addition & 1 deletion scripts/e2e.geth.instamine.sh
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ echo " "
# Launch client w/ two unlocked accounts.
# + accounts[0] default geth unlocked bal = ~infinity
# + accounts[1] unlocked, bal=50 eth, signing password = 'left-hand-of-darkness'
geth-dev-assistant --accounts 1 --tag 'v1.10.3'
geth-dev-assistant --accounts 1 --tag 'stable'

# Test
GETH_INSTAMINE=true nyc --no-clean --silent _mocha -- \
Expand Down
Loading