Skip to content

fix: transfer() rejected a fee exactly equal to transferMaxFee, unlike sendTransaction() - #125

Open
ayushsingh82 wants to merge 1 commit into
tetherto:mainfrom
ayushsingh82:fix/transfer-max-fee-boundary-consistency
Open

fix: transfer() rejected a fee exactly equal to transferMaxFee, unlike sendTransaction()#125
ayushsingh82 wants to merge 1 commit into
tetherto:mainfrom
ayushsingh82:fix/transfer-max-fee-boundary-consistency

Conversation

@ayushsingh82

Copy link
Copy Markdown

Summary

  • transfer()'s max-fee check used prepared.fee >= transferMaxFee, rejecting a fee exactly equal to the configured cap.
  • sendTransaction()/signTransaction() use > transactionMaxFee, allowing a fee exactly equal to the cap.
  • git blame shows the > boundary was a deliberate fix applied to transactionMaxFee (commit "updated max fee checks, testing") that was never mirrored to transferMaxFee, which was left behind with the original >=.
  • Concretely: call quoteTransfer(), pass its returned fee straight back in as transferMaxFee, and transfer() throws 'Exceeded maximum fee cost for transfer operation.' even though the fee didn't exceed the cap — it equaled it. The identical scenario through sendTransaction()/transactionMaxFee succeeds.

Change

  • src/wallet-account-evm-erc-4337.js: transfer()'s check changed from >= to >, matching sendTransaction()/signTransaction().
  • tests/wallet-account-evm-erc-4337.test.js: updated the existing test that had codified the old (inconsistent) behavior — 'should throw if the fee equals the transfer max fee configuration' — to assert the corrected, consistent behavior instead: 'should allow a fee exactly equal to the transfer max fee configuration', mirroring the equivalent sendTransaction test already in the suite.

Breaking change note

Per repo convention, flagging explicitly: this changes observable behavior of transfer() — a transfer whose fee exactly equals transferMaxFee will now succeed instead of throwing. No other public API surface changes.

Test plan

  • npm run lint (standard) passes
  • npm test passes — 92/92 unit tests (3 suites)
  • npm run build:types passes (generated .d.ts diff not included — it's drift from a different local tsc version across unrelated files, not caused by this change)
  • Confirmed the updated test previously passed against the old >= implementation (i.e. the old behavior was real, not a flaky test) before flipping it to assert the fixed behavior

…e sendTransaction()

transfer()'s max-fee check used `prepared.fee >= transferMaxFee`, rejecting a
fee exactly equal to the configured cap. sendTransaction()/signTransaction()
use `> transactionMaxFee`, allowing a fee exactly equal to the cap. Git blame
shows the `>` boundary was a deliberate fix applied to transactionMaxFee
(commit "updated max fee checks, testing") that was never mirrored to
transferMaxFee, left behind with the original `>=`.

Changed transfer()'s check to `>` to match. Updated the existing test that
had codified the old behavior ('should throw if the fee equals the transfer
max fee configuration') to assert the corrected, consistent behavior instead
('should allow a fee exactly equal to the transfer max fee configuration'),
mirroring the equivalent sendTransaction test.
@ayushsingh82

Copy link
Copy Markdown
Author

@jonathunne could you take a look when you have a chance? Thanks!

Summary of the fix: `transfer()`'s max-fee check used `prepared.fee >= transferMaxFee`, rejecting a fee exactly equal to the configured cap — while `sendTransaction()`/`signTransaction()` use `> transactionMaxFee`, allowing an exactly-equal fee. Git blame shows the `>` boundary was a deliberate fix applied to `transactionMaxFee` (commit "updated max fee checks, testing") that was never mirrored to `transferMaxFee`. Fixed `transfer()`'s check to `>` to match, and updated the existing test that had codified the old behavior. Flagging as a behavior change per repo convention: a transfer whose fee exactly equals `transferMaxFee` now succeeds instead of throwing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant