diff --git a/README.md b/README.md index 98485d7..2eae2d1 100644 --- a/README.md +++ b/README.md @@ -2075,8 +2075,10 @@ view would leave a wallet one click from deletion. artifact can be compared with it field for field. A nonce the site supplies is ignored: the nonce is always the account's next nonce from the node, so a site cannot replace one of the user's pending transactions or leave this one stuck - behind a gap. A gas limit the site supplies, as `gas` or `gasLimit`, is used - in place of the node's estimate; a request giving both with different values + behind a gap. A gas limit the site supplies, as `gas` or `gasLimit`, is the + one shown and signed in place of the node's estimate, but the node still + estimates the transaction with that limit, as the check that it neither + reverts nor runs out of gas; a request that gives both with different values is refused. A request that cannot be populated — unreachable node, reverting gas estimate — opens no window and is failed back to the site. Only one transaction approval exists at a time: populating fixes the nonce, so a second diff --git a/src/shared/approvalTx.js b/src/shared/approvalTx.js index 8bedffd..eb088ce 100644 --- a/src/shared/approvalTx.js +++ b/src/shared/approvalTx.js @@ -201,6 +201,7 @@ async function prepareApprovalTx(provider, from, txParams) { throw fail("There is no active address to send this transaction from."); } const request = requestFrom(txParams || {}, from); + const signer = new VoidSigner(getAddress(from), provider); let populated; try { @@ -208,10 +209,20 @@ async function prepareApprovalTx(provider, from, txParams) { // nonce, gas, fee and chain id are populated exactly as they were when // the popup did this. VoidSigner cannot sign, which is the point: the // background prepares, the popup signs. + // + // ethers asks the node for a gas estimate only when the request has + // no gas limit, and that estimate is the only check that the + // transaction does not revert. So a request carrying the site's gas + // limit is estimated first, with that limit: one that reverts, or + // needs more gas than the site gave it, fails here as well. The + // site's gas limit is still the one populated. populated = await withTimeout( - new VoidSigner(getAddress(from), provider).populateTransaction( - request, - ), + (async () => { + if (present(request.gasLimit)) { + await signer.estimateGas(request); + } + return signer.populateTransaction(request); + })(), POPULATE_TIMEOUT_MS, "The transaction could not be prepared: the network did not answer in time.", ); diff --git a/tests/approvalTx.test.js b/tests/approvalTx.test.js index 47955f0..4d8f14b 100644 --- a/tests/approvalTx.test.js +++ b/tests/approvalTx.test.js @@ -139,14 +139,20 @@ describe("prepareApprovalTx", () => { }); // `gas` is the JSON-RPC name for the gas limit and the one sites send. - // The node would estimate 21000; the site's 200000 is what is shown and - // what is signed. + // The node is still asked for its estimate, with the site's gas limit, + // because the estimate is the check that the transaction does not revert. + // It estimates 21000; the site's 200000 is what is shown and what is + // signed. test("uses the gas limit a site sends as gas", async () => { + const estimateGas = jest.fn(async () => 21000n); const approved = await prepareApprovalTx( - providerWith(), + providerWith({ estimateGas }), signer.address, { ...TX_PARAMS, gas: "0x30d40" }, ); + expect(estimateGas).toHaveBeenCalledWith( + expect.objectContaining({ gasLimit: 200000n }), + ); expect(approved.gasLimit).toBe("0x30d40"); const signed = Transaction.from( await signer.signTransaction({ ...approved }), @@ -297,6 +303,22 @@ describe("prepareApprovalTx", () => { expect(thrown.message).toMatch(/^[A-Z].*\.$/); }); + test("fails back a reverting request that sends its own gas limit", async () => { + const reverting = providerWith({ + estimateGas: async () => { + throw new Error("execution reverted: ERC20: transfer amount"); + }, + }); + await expect( + prepareApprovalTx(reverting, signer.address, { + ...TX_PARAMS, + gas: "0x30d40", + }), + ).rejects.toThrow( + /^The transaction could not be prepared: execution reverted/, + ); + }); + // Without a bound, an unreachable node leaves the page's promise pending // with nothing on screen to explain it. test("gives up on a node that never answers", async () => {