fix: estimate a request even when the site sends its gas limit (closes #554)
check / check (push) Canceled after 0s
e2e / e2e-chrome (push) Canceled after 0s
e2e / e2e-firefox (push) Canceled after 0s

ethers asks the node for a gas estimate only when the request has no gas
limit, and that estimate is the wallet's only check that a transaction
does not revert. Since a site's `gas` became the gas limit, a reverting
request carrying one opened the approval window and could be signed and
mined to fail. Such a request is now estimated first, with the site's
limit and under the same timeout; a failure goes back to the site with no
window, and the site's gas limit is still the one shown and signed. A
limit too low for the call now fails the same way.

Model: opus-5-5
This commit was merged in pull request #557.
This commit is contained in:
2026-10-14 19:46:01 +02:00
parent c40c7432a5
commit 575f1658ae
3 changed files with 65 additions and 8 deletions
+47 -3
View File
@@ -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 () => {
@@ -319,6 +341,28 @@ describe("prepareApprovalTx", () => {
jest.useRealTimers();
}
});
// The estimate of a request carrying the site's gas limit is a call of its
// own, made before populating, and is bounded the same way.
test("gives up on a node that never answers the estimate of a request that sends its own gas limit", async () => {
jest.useFakeTimers();
try {
const hanging = providerWith({
estimateGas: () => new Promise(() => {}),
});
const pending = prepareApprovalTx(hanging, signer.address, {
...TX_PARAMS,
gas: "0x30d40",
});
const settled = expect(pending).rejects.toThrow(
/did not answer in time/,
);
await jest.advanceTimersByTimeAsync(POPULATE_TIMEOUT_MS + 1);
await settled;
} finally {
jest.useRealTimers();
}
});
});
describe("serializeApprovedTx", () => {