feat: support fxUSD and multi-decimal EIP-3009 stablecoins#109
Open
huwangtao123 wants to merge 3 commits intoBlockRunAI:mainfrom
Open
feat: support fxUSD and multi-decimal EIP-3009 stablecoins#109huwangtao123 wants to merge 3 commits intoBlockRunAI:mainfrom
huwangtao123 wants to merge 3 commits intoBlockRunAI:mainfrom
Conversation
- Add paymentAssetDecimals to /health endpoint and proxy reuse path to prevent decimals data loss when reconnecting to an existing proxy (fixes 10^12x balance inflation for 18-decimal assets like fxUSD) - Fix transformPaymentError to use asset-native decimals instead of hardcoded 6 for USD conversion in insufficient_funds error messages - Update balance.ts and proxy.ts comments from USDC-specific to generic stablecoin language (USD micros) - Add payment-asset.ts module with BasePaymentAsset type, normalizer, and fetcher for multi-asset support - Add 3 new tests: fxUSD 18-decimal normalization, mixed-decimal asset list with disabled filtering, real fxUSD contract address Verified: fxUSD contract (0x55380fe7a1910dff29a47b622057ab4139da42c5) on Base fully implements EIP-3009 (transferWithAuthorization, all typehashes, authorizationState) — confirmed on Basescan.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBalance monitoring was generalized from USDC-only to configurable Base-chain stablecoins; a payment-asset module and tests were added; the proxy now tracks/advertises payment-asset metadata, selects a sufficient asset per request, and ties balance checks/deductions to the selected asset. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Proxy as Proxy Server
participant Fetcher as Asset Fetcher
participant Selector as Asset Selector
participant Monitor as BalanceMonitor
participant Chain as Blockchain
Client->>Proxy: incoming request (EVM/Base)
Proxy->>Fetcher: fetchBasePaymentAssets()
Fetcher-->>Proxy: normalized asset list
Proxy->>Selector: evaluate assets for sufficiency
loop for each candidate asset
Selector->>Monitor: setAsset(candidate)
Selector->>Monitor: checkSufficient(bufferedCostMicros)
Monitor->>Chain: read balance for candidate.asset
Chain-->>Monitor: raw balance
Monitor->>Monitor: toUsdMicros(raw balance, decimals)
Monitor-->>Selector: sufficiency result (assetSymbol, shortfall?)
alt sufficient
Selector-->>Proxy: select asset and stop
end
end
Proxy->>Proxy: assign requestBalanceMonitor & requestBasePaymentAsset
Proxy->>Monitor: optimistic deduction (USD micros)
Monitor-->>Proxy: deduction applied
Proxy-->>Client: proceed / respond
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 (2)
src/proxy.ts (1)
1568-1577: Async asset refresh inonAfterPaymentCreationmay race with concurrent requests.This callback fetches assets and updates
activeBasePaymentAssetsandactiveBasePaymentAssetwhile other requests may be reading these variables in their asset selection loops (lines 3438, 3447).Since the selection loop creates its own monitors and reads
getBasePaymentAssets()at the start of selection, this should be safe — a request either sees the old list or the new list, both valid. However, consider documenting this non-atomic update if it's intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy.ts` around lines 1568 - 1577, The async refresh in onAfterPaymentCreation can non-atomically update activeBasePaymentAssets/activeBasePaymentAsset while other requests read them; fetch the assets into a local variable first (e.g., const newAssets = await fetchBasePaymentAssets(apiBase).catch(() => undefined)), then if newAssets is truthy assign activeBasePaymentAssets = newAssets and activeBasePaymentAsset = newAssets[0] ?? activeBasePaymentAsset, and only after that call balanceMonitor.setAsset(activeBasePaymentAsset) (BalanceMonitor); alternatively add a brief comment near getBasePaymentAssets()/onAfterPaymentCreation documenting that updates are intentionally non-atomic and readers accept either the old or new list.src/payment-asset.ts (1)
99-101: Consider handling JSON parse errors infetchBasePaymentAssets.If the API returns a 200 OK with invalid JSON,
response.json()will throw and the error propagates up. This could cause unexpected failures when the upstream API has transient issues.🛡️ Suggested defensive handling
if (!response.ok) return [DEFAULT_BASE_PAYMENT_ASSET]; - const payload = (await response.json()) as PaymentMetadataResponse; - return normalizeBasePaymentAssets(payload); + try { + const payload = (await response.json()) as PaymentMetadataResponse; + return normalizeBasePaymentAssets(payload); + } catch { + return [DEFAULT_BASE_PAYMENT_ASSET]; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/payment-asset.ts` around lines 99 - 101, fetchBasePaymentAssets currently calls response.json() directly which will throw on invalid JSON and propagate; wrap the JSON parsing in a try/catch inside fetchBasePaymentAssets, attempt to read and include the raw body (e.g., response.text()) or the parse error message in the catch, log or attach that context, and then either throw a clearer, typed error or return a safe fallback before calling normalizeBasePaymentAssets(payload); reference fetchBasePaymentAssets, response.json(), payload, and normalizeBasePaymentAssets to locate where to add the try/catch and error enrichment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/balance.ts`:
- Around line 163-172: setAsset mutates BalanceMonitor while async balance
checks (checkSufficient/checkBalance/fetchBalance) can be in-flight, causing
cached balances or responses to be associated with the wrong asset; make
BalanceMonitor immutable instead of mutating it: remove/stop using setAsset and
ensure callers (e.g., the onAfterPaymentCreation path that currently calls
setAsset) create a new BalanceMonitor instance per asset (or per asset change)
so each monitor owns a fixed BasePaymentAsset, or alternatively implement an
atomic/version check in fetchBalance that aborts/cancels caching if this.asset
changed during the fetch; reference: setAsset, BalanceMonitor, checkSufficient,
checkBalance, fetchBalance, onAfterPaymentCreation.
---
Nitpick comments:
In `@src/payment-asset.ts`:
- Around line 99-101: fetchBasePaymentAssets currently calls response.json()
directly which will throw on invalid JSON and propagate; wrap the JSON parsing
in a try/catch inside fetchBasePaymentAssets, attempt to read and include the
raw body (e.g., response.text()) or the parse error message in the catch, log or
attach that context, and then either throw a clearer, typed error or return a
safe fallback before calling normalizeBasePaymentAssets(payload); reference
fetchBasePaymentAssets, response.json(), payload, and normalizeBasePaymentAssets
to locate where to add the try/catch and error enrichment.
In `@src/proxy.ts`:
- Around line 1568-1577: The async refresh in onAfterPaymentCreation can
non-atomically update activeBasePaymentAssets/activeBasePaymentAsset while other
requests read them; fetch the assets into a local variable first (e.g., const
newAssets = await fetchBasePaymentAssets(apiBase).catch(() => undefined)), then
if newAssets is truthy assign activeBasePaymentAssets = newAssets and
activeBasePaymentAsset = newAssets[0] ?? activeBasePaymentAsset, and only after
that call balanceMonitor.setAsset(activeBasePaymentAsset) (BalanceMonitor);
alternatively add a brief comment near
getBasePaymentAssets()/onAfterPaymentCreation documenting that updates are
intentionally non-atomic and readers accept either the old or new list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c0df0cb-eb1f-4015-afc0-1c3c0b4d7eab
📒 Files selected for processing (4)
src/balance.tssrc/payment-asset.test.tssrc/payment-asset.tssrc/proxy.ts
Comment on lines
+163
to
+172
| setAsset(asset: BasePaymentAsset): void { | ||
| if ( | ||
| this.asset.asset.toLowerCase() !== asset.asset.toLowerCase() || | ||
| this.asset.symbol !== asset.symbol || | ||
| this.asset.decimals !== asset.decimals | ||
| ) { | ||
| this.asset = asset; | ||
| this.invalidate(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential race condition when setAsset() is called concurrently with balance checks.
Per the context snippets, setAsset() is called from onAfterPaymentCreation (async callback during payment creation) while checkSufficient() may be executing concurrently for other requests. The check-then-act pattern at lines 164-170 is not atomic with respect to async operations:
- Request A calls
checkBalance()→ awaitsfetchBalance()with asset X - Request B's payment triggers
onAfterPaymentCreation→setAsset(Y)invalidates cache - Request A's
fetchBalance()returns balance for asset X, butthis.assetis now Y
This could cause balance info to report the wrong assetSymbol or cache a balance for the wrong asset.
Consider either:
- Making
BalanceMonitorimmutable (create new instance per asset) — whichsrc/proxy.tsalready does in the asset selection loop at lines 3447-3457 - Or documenting that
setAsset()should only be called when no concurrent operations are in flight
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/balance.ts` around lines 163 - 172, setAsset mutates BalanceMonitor while
async balance checks (checkSufficient/checkBalance/fetchBalance) can be
in-flight, causing cached balances or responses to be associated with the wrong
asset; make BalanceMonitor immutable instead of mutating it: remove/stop using
setAsset and ensure callers (e.g., the onAfterPaymentCreation path that
currently calls setAsset) create a new BalanceMonitor instance per asset (or per
asset change) so each monitor owns a fixed BasePaymentAsset, or alternatively
implement an atomic/version check in fetchBalance that aborts/cancels caching if
this.asset changed during the fetch; reference: setAsset, BalanceMonitor,
checkSufficient, checkBalance, fetchBalance, onAfterPaymentCreation.
Adds docstrings to all exported types, constants, and functions to satisfy the 80% docstring coverage CI requirement.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/payment-asset.ts`:
- Around line 113-124: fetchBasePaymentAssets currently doesn't catch network or
JSON parse errors, so add a try-catch around the fetch + json + normalize steps
in fetchBasePaymentAssets and, on any thrown error, log or ignore the error and
return [DEFAULT_BASE_PAYMENT_ASSET] as the documented fallback; ensure you still
preserve the existing behavior for non-ok responses (the early return when
response.ok is false) and reference the PaymentMetadataResponse parsing and
normalizeBasePaymentAssets call inside the try block so parse/normalize failures
also fall back.
- Around line 87-88: The function normalizeBasePaymentAssets currently returns
[] early when value is null/undefined or not an object, which bypasses the
fallback-to-USDC logic; modify normalizeBasePaymentAssets so it does not
early-return for falsy/non-object inputs but instead allows the existing
fallback path (the code that adds USDC when no valid assets are found) to
run—locate normalizeBasePaymentAssets and remove or change the early return
check so invalid input is handled by the later validation/fallback logic that
produces the USDC default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8345e67-07ef-4b45-8ae2-f2ee7023d2c2
📒 Files selected for processing (1)
src/payment-asset.ts
- normalizeBasePaymentAssets now returns [DEFAULT_BASE_PAYMENT_ASSET] instead of [] for null/undefined/non-object input, ensuring the USDC fallback is always applied - fetchBasePaymentAssets wraps fetch+json+normalize in try-catch so network errors and JSON parse failures fall back to USDC instead of propagating (matches documented contract)
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.
What
Add proper multi-decimal EIP-3009 stablecoin support, fixing bugs that would cause incorrect behavior with non-6-decimal assets like fxUSD (18 decimals).
Why
ClawRouter's payment layer was hardcoded to assume 6 decimals (USDC) in several places. Adding fxUSD (18 decimals, verified on Basescan) exposed these gaps.
Changes
/healthendpoint now returnspaymentAssetDecimalsdecimalsfrom existing proxy (fixes 10^12× balance inflation for 18-decimal assets)transformPaymentErroruses asset-native decimals for USD conversion ininsufficient_fundserrorspayment-asset.ts(new) —BasePaymentAssettype, normalizer, fetcherbalance.ts— updated USDC-specific comments to generic stablecoinOn-Chain Verification
fxUSD contract (
0x55380fe7a1910dff29a47b622057ab4139da42c5) on Base implements full EIP-3009:transferWithAuthorization✅receiveWithAuthorization✅cancelAuthorization✅authorizationState+DOMAIN_SEPARATOR✅Testing
Summary by CodeRabbit