-
Notifications
You must be signed in to change notification settings - Fork 87
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
Support EIP-1559 transaction #177
base: main
Are you sure you want to change the base?
Changes from 1 commit
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 |
---|---|---|
|
@@ -33,10 +33,13 @@ async function writeFactoryDeployerTransaction(contract: CompilerOutputContract, | |
const data = arrayFromHexString(deploymentBytecode) | ||
|
||
if (!process.env.MNEMONIC) throw Error("MNEMONIC is required") | ||
const signer = ethers.Wallet.fromMnemonic(process.env.MNEMONIC!!) | ||
const signedEncodedTransaction = await signer.signTransaction({ | ||
nonce, gasPrice, gasLimit, value, data, chainId | ||
}) | ||
const provider = new ethers.providers.JsonRpcProvider(process.env.RPC) | ||
const signer = ethers.Wallet.fromMnemonic(process.env.MNEMONIC!!).connect(provider) | ||
//if the network supports EIP-1155 transaction, use the EIP-1155 transaction type | ||
const tx = await signer.populateTransaction({ | ||
nonce, gasPrice, gasLimit, value, data, chainId | ||
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. How are you using EIP-1559 here? You haven't specified the transaction type and you are not using the maxPriorityFeePerGas or maxBaseFeePerGas here, right 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 was also confused for a while, but the key here seems to be using the Also, possibly we'd need to support multiple transaction types because, like networks only supporting eip-1559 transactions, there are networks that only support legacy ones. 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 see, didn't know the populateTransaction method does that by default 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. The function // Populates all fields in a transaction, signs it and sends it to the network
async sendTransaction(transaction: Deferrable<TransactionRequest>): Promise<TransactionResponse> {
this._checkProvider("sendTransaction");
const tx = await this.populateTransaction(transaction);
const signedTx = await this.signTransaction(tx);
return await this.provider.sendTransaction(signedTx);
} Before signing the transaction, it calls the The current adaptation is made to align with the default behavior of the ethers.js library in order to support all EVM-compatible chains. 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.
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 apologize for missing my point. There are several methods of doing things to accomplish the goals in mind, and where I really was going with my comment was that the use of: |
||
}) | ||
const signedEncodedTransaction = await signer.signTransaction(tx) | ||
const signerAddress = await signer.getAddress() | ||
const contractAddress = ethers.utils.getContractAddress({ from: signerAddress, nonce } ) | ||
|
||
|
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.
the comment seems to mention the incorrect eip number
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.
thank you for your review, it's fixed now.