-
Notifications
You must be signed in to change notification settings - Fork 5k
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
EIP1559 #4155
Changes from 31 commits
ec47e41
d50f17a
87738b7
059be6d
7b2acdf
bc8a427
5c11080
c53a87e
f963659
3e4fcbe
7681019
3212c05
43a755f
51f030e
3b3efb2
b458b63
1b66314
ad5a22e
4ebd78a
cd490a8
43fc7aa
5c5a517
34e5cf6
72c9898
073be03
1e3d662
2bb8c63
572e416
abbc390
ef39538
494f260
2437073
b3ab709
dc810b3
e7d3833
d9c509a
2a90980
7324eec
d4f1363
f2ca0d8
e0e19f7
c5be542
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
}] | ||
}), | ||
]; | ||
// attach methods to this._ethereumCall | ||
this._ethereumCall = {}; | ||
|
@@ -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) { | ||
|
@@ -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; | ||
|
@@ -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; | ||
|
@@ -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 | ||
}); | ||
}); | ||
}; | ||
|
||
|
@@ -272,20 +297,117 @@ function _validateTransactionForSigning(tx) { | |
); | ||
} | ||
|
||
if (!tx.gas && !tx.gasLimit) { | ||
if ( | ||
(!tx.gas && !tx.gasLimit) && | ||
(!tx.maxPriorityFeePerGas && !tx.maxFeePerGas) | ||
) { | ||
return new Error('"gas" is missing'); | ||
} | ||
|
||
if (tx.nonce < 0 || | ||
tx.gas < 0 || | ||
tx.gasPrice < 0 || | ||
tx.chainId < 0) { | ||
if (tx.gas && tx.gasPrice) { | ||
if (tx.gas < 0 || tx.gasPrice < 0) { | ||
return new Error('Gas, gasPrice, nonce or chainId is lower than 0'); | ||
spacesailor24 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} else { | ||
if (tx.maxPriorityFeePerGas < 0 || tx.maxFeePerGas < 0) { | ||
return new Error('maxPriorityFeePerGas or maxFeePerGas is lower than 0'); | ||
} | ||
} | ||
|
||
if (tx.nonce < 0 || tx.chainId < 0) { | ||
return new Error('Gas, gasPrice, nonce or chainId is lower than 0'); | ||
spacesailor24 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
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' && | ||
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. 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? 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. So while this is double checking Here we are short circuiting and returning And here we are verifying we aren't of type 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 | ||
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. 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. 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. This was something that was suggested on a call with wallet/library implementers, but we should make a note in the docs 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. We'll add |
||
maxFeePerGas = tx.maxFeePerGas || | ||
utils.toHex( | ||
utils.toBN(block.baseFeePerGas) | ||
.mul(utils.toBN(2)) | ||
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. 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 | ||
|
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.
is this a new method?
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.
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 thebaseFeePerGas
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.
You mean we can't leverage the getBlock (also refs blockCall) because it's defined in the
web3-eth
package and not accessible fromweb3-eth-accounts
?In this case don't we have a dependency or at least a DRY issue that could be addressed somehow?
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.
@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 to1.x
as possible, to refrain from introducing additional bugs