Add optional upto verify balance preflight and error mapping#40
Merged
ponderingdemocritus merged 1 commit intomainfrom Feb 18, 2026
Merged
Conversation
📝 WalkthroughWalkthroughA new optional balance preflight check feature is added to the Upto (batched) payment verification flow via environment variable and config. The enhancement enables on-chain balance validation during payment verification before settlement, complete with new ABI support, error handling, and comprehensive test coverage. Changes
Sequence DiagramsequenceDiagram
participant Client
participant UptoEvmScheme
participant SignatureVerifier as Signature<br/>Verifier
participant ERC20Contract as ERC-20<br/>Contract
participant Settlement as Settlement<br/>Handler
Client->>UptoEvmScheme: verify(paymentRequest)
UptoEvmScheme->>SignatureVerifier: Verify permit signature
alt Signature Invalid
SignatureVerifier-->>UptoEvmScheme: invalid_permit_signature
UptoEvmScheme-->>Client: {valid: false, reason}
else Signature Valid
SignatureVerifier-->>UptoEvmScheme: signature valid
alt verifyBalanceCheck enabled
UptoEvmScheme->>ERC20Contract: readContract(balanceOf, owner)
alt Balance check passes
ERC20Contract-->>UptoEvmScheme: balance ≥ amount
UptoEvmScheme-->>Client: {valid: true}
else Insufficient balance
ERC20Contract-->>UptoEvmScheme: balance < amount
UptoEvmScheme-->>Client: {valid: false, reason: insufficient_balance}
else RPC failure
ERC20Contract--xUptoEvmScheme: error
UptoEvmScheme-->>Client: {valid: false, reason: balance_check_failed}
end
else verifyBalanceCheck disabled
UptoEvmScheme-->>Client: {valid: true}
end
end
Client->>Settlement: settle(paymentRequest)
Settlement->>Settlement: Execute transferFrom
alt Transfer succeeds
Settlement-->>Client: {success: true}
else Transfer fails
Settlement->>Settlement: mapTransferErrorReason(error)
alt Low balance
Settlement-->>Client: {success: false, reason: insufficient_balance}
else Low allowance
Settlement-->>Client: {success: false, reason: insufficient_allowance}
else Other error
Settlement-->>Client: {success: false, reason: transaction_failed}
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/core/src/upto/evm/verification.ts (1)
241-247: Consider logging the RPC error before returningbalance_check_failed.The catch block silently discards the error. For operators debugging why verifications are failing, it would help to log the underlying RPC error (similar to how
settlement.tsline 191 logs the error before returning).Proposed change
- } catch { + } catch (error) { + console.error("Balance preflight failed:", errorSummary(error)); return { isValid: false, invalidReason: "balance_check_failed", payer, }; }This would require importing
errorSummaryfrom./constants.js(already partially imported on line 15).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/upto/evm/verification.ts` around lines 241 - 247, The catch in the balance check in verifyPayer (or the surrounding verification function in packages/core/src/upto/evm/verification.ts) silently discards the RPC error; import errorSummary from ./constants.js (it's already partially imported) and call processLogger.error (or the same logger used in this file) with a concise message and errorSummary(err) inside that catch before returning the { isValid: false, invalidReason: "balance_check_failed", payer } object so operators can see the underlying RPC failure.packages/core/src/factory.ts (1)
53-62: Consider usingUptoEvmSchemeOptionstype instead of inlining the shape.The inline type
{ verifyBalanceCheck?: boolean }is structurally identical toUptoEvmSchemeOptionstoday, but ifUptoEvmSchemeOptionsgains additional fields in the future, this inline definition would silently drift out of sync, requiring manual updates in two places.Proposed change
+import type { UptoEvmSchemeOptions } from "./upto/evm/facilitator.js"; export interface EvmSignerConfig { // ... - upto?: { - verifyBalanceCheck?: boolean; - }; + upto?: UptoEvmSchemeOptions; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/factory.ts` around lines 53 - 62, The upto property in the exported options is currently typed inline as `{ verifyBalanceCheck?: boolean }`, which duplicates the shape of UptoEvmSchemeOptions; replace the inline type with the shared UptoEvmSchemeOptions type (use upto?: UptoEvmSchemeOptions) so the property stays in sync if that type evolves; update any imports/exports in this file to reference UptoEvmSchemeOptions and ensure type usage in functions like the factory/constructor that read upto still type-checks after the change.packages/core/tests/unit/uptoEvmScheme.test.ts (1)
395-472: Consider adding a test that confirmsreadContractis never called whenverifyBalanceCheckis absent (default).The four new tests confirm preflight behavior when the flag is
trueand verify short-circuit ordering. However, there is no test that proves the feature is strictly opt-in — i.e., that the defaultnew UptoEvmScheme(mockSigner)path never invokesreadContractduringverify. Without that test, a regression that callsreadContractunconditionally would go undetected.💡 Suggested additional test
it("does not run balance preflight when verifyBalanceCheck is not set", async () => { const readContractMock = mock(() => Promise.resolve(1000000n)); mockSigner = createMockSigner({ readContract: readContractMock }); // Default scheme — no options scheme = new UptoEvmScheme(mockSigner); const payload = createValidPayload(); const requirements = createValidRequirements(); const result = await scheme.verify(payload, requirements); expect(result.isValid).toBe(true); expect(readContractMock).not.toHaveBeenCalled(); });Would you like me to open an issue to track adding this coverage?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/tests/unit/uptoEvmScheme.test.ts` around lines 395 - 472, Add a unit test to assert that balance preflight is opt-in: instantiate UptoEvmScheme with no options (i.e., omit verifyBalanceCheck), supply a mock signer whose readContract is a jest/mock function (e.g., readContractMock created via createMockSigner), call scheme.verify(payload, requirements) using createValidPayload/createValidRequirements, assert result.isValid is true and that readContractMock was not called; reference UptoEvmScheme, verifyBalanceCheck, verify, readContract, createMockSigner, createValidPayload, and createValidRequirements to locate where to add this test.examples/facilitator-server/src/setup.ts (1)
112-112:uptoobject is always emitted even whenverifyBalanceCheckisfalse.When
UPTO_VERIFY_BALANCE_CHECKisfalse, passingupto: { verifyBalanceCheck: false }is equivalent to omitting the key (assuming the core treatsundefinedandfalseidentically). Consider conditionally spreading it to make the opt-in nature explicit at the call site.♻️ Conditional spread
- upto: { verifyBalanceCheck: UPTO_VERIFY_BALANCE_CHECK }, + ...(UPTO_VERIFY_BALANCE_CHECK ? { upto: { verifyBalanceCheck: true } } : {}),Apply the same change on line 156.
Also applies to: 156-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/facilitator-server/src/setup.ts` at line 112, The current call always emits an upto object with verifyBalanceCheck set to UPTO_VERIFY_BALANCE_CHECK even when false; update the callsite so the upto key is only included when UPTO_VERIFY_BALANCE_CHECK is truthy by conditionally spreading the property (e.g. use ...(UPTO_VERIFY_BALANCE_CHECK ? { upto: { verifyBalanceCheck: UPTO_VERIFY_BALANCE_CHECK } } : {}) or equivalent) instead of always passing upto: { verifyBalanceCheck: UPTO_VERIFY_BALANCE_CHECK }, and apply the same conditional-spread change at the other occurrence referenced around line 156; keep references to UPTO_VERIFY_BALANCE_CHECK and the upto object to locate the spots (same change at both call sites).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/tests/unit/uptoEvmScheme.test.ts`:
- Around line 706-729: The test failure stems from mapTransferErrorReason in
settlement.ts only matching two string patterns, so extend its matching logic to
also inspect decoded ABI error names and additional variants: check error.name
(or decodedError.name) for "ERC20InsufficientBalance" (and other ERC20-style
error names), perform case-insensitive contains checks for both "transfer amount
exceeds balance" and "insufficient balance" without relying on an "ERC20:"
prefix, and as a fallback detect token-related insufficiency by searching for
both "insufficient" and "balance" tokens in the error message; update any tests
referencing mapTransferErrorReason or UptoEvmScheme.settle expectations
accordingly.
---
Nitpick comments:
In `@examples/facilitator-server/src/setup.ts`:
- Line 112: The current call always emits an upto object with verifyBalanceCheck
set to UPTO_VERIFY_BALANCE_CHECK even when false; update the callsite so the
upto key is only included when UPTO_VERIFY_BALANCE_CHECK is truthy by
conditionally spreading the property (e.g. use ...(UPTO_VERIFY_BALANCE_CHECK ? {
upto: { verifyBalanceCheck: UPTO_VERIFY_BALANCE_CHECK } } : {}) or equivalent)
instead of always passing upto: { verifyBalanceCheck: UPTO_VERIFY_BALANCE_CHECK
}, and apply the same conditional-spread change at the other occurrence
referenced around line 156; keep references to UPTO_VERIFY_BALANCE_CHECK and the
upto object to locate the spots (same change at both call sites).
In `@packages/core/src/factory.ts`:
- Around line 53-62: The upto property in the exported options is currently
typed inline as `{ verifyBalanceCheck?: boolean }`, which duplicates the shape
of UptoEvmSchemeOptions; replace the inline type with the shared
UptoEvmSchemeOptions type (use upto?: UptoEvmSchemeOptions) so the property
stays in sync if that type evolves; update any imports/exports in this file to
reference UptoEvmSchemeOptions and ensure type usage in functions like the
factory/constructor that read upto still type-checks after the change.
In `@packages/core/src/upto/evm/verification.ts`:
- Around line 241-247: The catch in the balance check in verifyPayer (or the
surrounding verification function in packages/core/src/upto/evm/verification.ts)
silently discards the RPC error; import errorSummary from ./constants.js (it's
already partially imported) and call processLogger.error (or the same logger
used in this file) with a concise message and errorSummary(err) inside that
catch before returning the { isValid: false, invalidReason:
"balance_check_failed", payer } object so operators can see the underlying RPC
failure.
In `@packages/core/tests/unit/uptoEvmScheme.test.ts`:
- Around line 395-472: Add a unit test to assert that balance preflight is
opt-in: instantiate UptoEvmScheme with no options (i.e., omit
verifyBalanceCheck), supply a mock signer whose readContract is a jest/mock
function (e.g., readContractMock created via createMockSigner), call
scheme.verify(payload, requirements) using
createValidPayload/createValidRequirements, assert result.isValid is true and
that readContractMock was not called; reference UptoEvmScheme,
verifyBalanceCheck, verify, readContract, createMockSigner, createValidPayload,
and createValidRequirements to locate where to add this test.
Comment on lines
+706
to
+729
| it("returns insufficient_balance when transferFrom reverts for low balance", async () => { | ||
| const writeContractMock = mock() | ||
| .mockImplementationOnce(() => | ||
| Promise.resolve("0xpermittx" as `0x${string}`) | ||
| ) | ||
| .mockImplementationOnce(() => | ||
| Promise.reject( | ||
| new Error("ERC20: transfer amount exceeds balance") | ||
| ) | ||
| ); | ||
|
|
||
| mockSigner = createMockSigner({ | ||
| writeContract: writeContractMock, | ||
| }); | ||
| scheme = new UptoEvmScheme(mockSigner); | ||
|
|
||
| const payload = createValidPayload(); | ||
| const requirements = createValidRequirements(); | ||
|
|
||
| const result = await scheme.settle(payload, requirements); | ||
|
|
||
| expect(result.success).toBe(false); | ||
| expect(result.errorReason).toBe("insufficient_balance"); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "transfer amount exceeds balance|insufficient.balance|ERC20Insufficient|errorReason" \
--type ts --type tsx -C 3Repository: daydreamsai/facilitator
Length of output: 94
🏁 Script executed:
#!/bin/bash
# First locate settlement.ts file
fd -t f "settlement" --type f
# Then search for error matching patterns
rg -n "transfer amount exceeds balance|insufficient.balance|ERC20Insufficient|errorReason" \
--type ts -C 3Repository: daydreamsai/facilitator
Length of output: 28216
🏁 Script executed:
# Find the errorSummary function definition
rg -n "errorSummary" --type ts -A 5 -B 2Repository: daydreamsai/facilitator
Length of output: 7853
🏁 Script executed:
cat -n packages/core/src/upto/evm/constants.ts | sed -n '95,130p'Repository: daydreamsai/facilitator
Length of output: 806
🏁 Script executed:
# Check if codebase mentions viem's custom error handling or OZ v5
rg -n "custom.error|ERC20InsufficientBalance|viem.*error|decodeError" --type ts -A 2Repository: daydreamsai/facilitator
Length of output: 403
🏁 Script executed:
# Search for any OZ v5 references, custom error handling, or viem imports
rg -n "OpenZeppelin|v5|custom.?error|viem" --type ts packages/core/src --max-count 20Repository: daydreamsai/facilitator
Length of output: 1440
🏁 Script executed:
# Check how writeContract is being used and what error handling exists
rg -n "writeContract|ContractError" --type ts packages/core/tests/unit/uptoEvmScheme.test.ts -B 2 -A 3Repository: daydreamsai/facilitator
Length of output: 4498
🏁 Script executed:
# Check what erc20Abi contains - look for its definition in constants.ts
rg -n "erc20Abi" --type ts packages/core/src/upto/evm/constants.ts -A 50 | head -80Repository: daydreamsai/facilitator
Length of output: 1472
🏁 Script executed:
# Check if there are any error definitions in the erc20Abi or any custom error handling
rg -n "error.*type|CustomError|ERC20" --type ts packages/core/src/upto/evm -A 2Repository: daydreamsai/facilitator
Length of output: 314
🏁 Script executed:
# Look for any test cases that show what actual error messages look like
rg -n "shortMessage|message.*ERC20\|ERC20.*message" --type ts packages/core/tests -B 2 -A 2Repository: daydreamsai/facilitator
Length of output: 1607
String-based error matching may miss OpenZeppelin v5+ and other ERC-20 variants.
The test covers the OZ v4 revert string "ERC20: transfer amount exceeds balance". The settlement code uses string pattern matching via mapTransferErrorReason(), which checks only two patterns: "transfer amount exceeds balance" and "insufficient balance" (lines 25–31 in settlement.ts). OpenZeppelin v5 emits ERC20InsufficientBalance as an ABI-encoded custom error, which viem may decode into a different message format that would not match these patterns, causing it to surface as transaction_failed rather than insufficient_balance.
Expand the error mapping to catch:
- ABI-decoded custom error names if viem provides them (e.g.,
"ERC20InsufficientBalance") - Common variants without the
"ERC20:"prefix (e.g.,"transfer amount exceeds balance") - Any other ERC-20 implementations that use different message formats
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/tests/unit/uptoEvmScheme.test.ts` around lines 706 - 729, The
test failure stems from mapTransferErrorReason in settlement.ts only matching
two string patterns, so extend its matching logic to also inspect decoded ABI
error names and additional variants: check error.name (or decodedError.name) for
"ERC20InsufficientBalance" (and other ERC20-style error names), perform
case-insensitive contains checks for both "transfer amount exceeds balance" and
"insufficient balance" without relying on an "ERC20:" prefix, and as a fallback
detect token-related insufficiency by searching for both "insufficient" and
"balance" tokens in the error message; update any tests referencing
mapTransferErrorReason or UptoEvmScheme.settle expectations accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds configurable upto verify balance wiring across facilitator config, scheme registration, and the facilitator-server env var UPTO_VERIFY_BALANCE_CHECK. It adds optional post-signature on-chain balanceOf(owner) preflight in upto verify with fail-closed balance_check_failed behavior and early insufficient_balance when underfunded. Settlement now maps transfer revert text to insufficient_balance or insufficient_allowance instead of always returning transaction_failed. Tests were expanded for balance preflight success/failure/order semantics and transfer error mapping, and the README/server docs were updated with configuration and API behavior details.
Summary by CodeRabbit
New Features
Documentation