feat(pages): per-chain page lookups + decimal-aware approve/lock flow#11
feat(pages): per-chain page lookups + decimal-aware approve/lock flow#11fielding wants to merge 4 commits into
Conversation
Replaces the default-chain BROKER_FEE singleton with BROKER_FEE_RATE and two helpers — brokerFeeForTreasury(treasury) and brokerFeePctString(fee) — so each call site can compute the broker fee from its active chain's treasury rather than the deployment's default. Addresses the third review note on PR #8. parseAmountParam now takes an optional decimals cap (default 6) so BNB Chain's 18-decimal USDC isn't truncated by the URL-param sanity regex. The default keeps every existing call site unchanged. The chain-specific singleton exports (SABLIER_LOCKUP, USDC_ADDRESS, TREASURY, EXPLORER_URL, STREAM_START_BLOCK, LOG_CHUNK_SIZE) are removed in this commit; the next commit switches every consumer over to getChainConfig(useChainId()). Refs: RG-9430aa, RG-42e939 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switches /create, /vaults, and the landing page from default-chain singletons to getChainConfig(useChainId()) lookups. Every per-chain value — Sablier address, USDC address, USDC decimals, treasury, explorer URL, broker fee, stream start block, log chunk size — now flows from the active chain's registry entry. The minimum lock amount, formerly a hardcoded 1_000_000 (1 USDC at 6 decimals), is now BigInt(10) ** BigInt(usdcDecimals) so BNB's 18-decimal USDC also enforces a 1-USDC floor (1e18 base units). Sub-components (VestingCalculator, TimelinePreview, ConfirmDialog, SuccessView, VaultCard) take the chain-derived values as props rather than reading module-level singletons. parseStreamIdFromReceipt takes sablierAddress as a parameter. The vaults indexer query parameterizes chainId from useChainId() instead of an env-derived constant, and fetchFromChain takes the chain config so the on-chain fallback uses the right Sablier address, start block, and chunk size. The landing's "view contract" link reads from the active chain via useChainId() — unconnected users still see the wagmi default chain's contract, which on the mainnet site is Base. No behavior change on Base mainnet or Base Sepolia. Tests + typecheck + production build all clean. Refs: RG-9430aa, RG-42e939 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer orientation — multi-EVM stack [2/6]This PR is part of a 6-PR stacked multi-EVM rollout. Review #8 first if you haven't — this builds on its registry.
Move to #12 when done. Heads up: this PR introduces a regression where users on unsupported chains crash on `getChainConfig()` — that's fixed in #12's wrong-chain guard. Don't merge this one without #12 right behind it. |
fielding
left a comment
fielding
left a comment
There was a problem hiding this comment.
Requesting changes on this layer.
The refactor makes the tx flow chain-aware, but several callbacks/effects still close over chain-specific values without listing them as deps. pnpm --filter app lint flags the important ones in /create and /vaults:
depositAmountdepends onusdcDecimals.feedepends onbrokerFee.handleApproveusesusdcAddressandsablierLockup.handleLockand the approve-to-lock effect usesablierLockup,usdcAddress,treasury,brokerFee, andusdcDecimals.- The lock success effect uses
explorerUrl,sablierLockup, andusdcDecimals. - The vault claim callback/effect use
sablierLockupandexplorerUrl.
This is not just lint noise now that the page can move between supported chains while mounted. A user can start an approve/lock flow on one chain, switch networks, and have UI state and callbacks disagree about token, Sablier address, treasury, fee, decimals, or explorer URL. The delayed approve-to-lock timeout is the riskiest path.
Please fix the deps and add a guard for network changes during an active tx flow. I would either:
useEffect(() => {
if (step !== "schedule" && step !== "success") {
resetForm();
toast("Network changed. Review the lock again on the new chain.", "error");
}
}, [chainId]);or store a flowChainId when the user enters confirm/approve/lock and refuse to continue if the current chain no longer matches.
One smaller follow-up while you are in here: the remembered schedule label key is only ripguard:lock:${streamId}. Stream IDs can collide across chains/contracts, so the key should include chainId or the Sablier address.
Validation I ran on the top of the stack:
pnpm --filter app test
pnpm --filter app exec tsc --noEmit
NODE_ENV=production pnpm --filter app build
pnpm --filter app lintTests, typecheck, and build passed. Lint fails overall because of pre-existing React 19 rules too, but the missing-dependency warnings above are introduced by this refactor and are real for multi-chain behavior.
…d labels Addresses review feedback on #11. 1. Missing useEffect/useCallback deps for chain-derived values are now listed: usdcDecimals, brokerFee, sablierLockup, usdcAddress, treasury, explorerUrl. Without these, a chain switch mid-render could leave callbacks closed over stale values from the previous chain — most dangerously the approve→lock priming timeout, which would have fired writeLock against the old chain's Sablier address. 2. Chain-change guard: if useChainId() flips while step is past schedule (confirm/approve/lock), reset the form and toast the user. Stops the cross-chain race entirely. Uses a lastChainIdRef so the effect only acts on actual chain transitions, not initial mount. (The dep array includes step/resetForm/toast for lint, but the ref short-circuit means the body only runs when chainId changed.) 3. localStorage label key now namespaced by chainId: `ripguard:lock:<chainId>:<streamId>`. Sablier stream IDs reset per chain — without this, stream #5 on Arbitrum would steal the schedule label of stream #5 on Base. getLockLabel takes chainId as a parameter; VaultCard receives it as a prop from VaultDashboard. Existing locks under the old key fall back to the generic schedule type on /vaults — acceptable, since the fallback path was already in place. Refs: RG-9430aa (review feedback) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Pushed fixes addressing your changes-requested review: Missing deps — added the chain-derived values to every callback/effect dep array you flagged:
Chain-change guard — went with the resetForm approach you suggested. Uses a `lastChainIdRef` so the effect body only fires on actual chainId transitions (not on initial mount or unrelated step changes). Toasts the user, clears the form, returns to the schedule step. Closes the approve→lock priming-timeout race entirely. localStorage label key — namespaced by chainId now: `ripguard:lock::`. `getLockLabel` takes `chainId` as a parameter; `VaultCard` receives it as a prop. Existing locks under the old key fall back to the generic schedule type — acceptable since the fallback path was already there. Tests + typecheck + production build pass. Ready for re-review. |
fielding
left a comment
fielding
left a comment
There was a problem hiding this comment.
Re-reviewed after the fix commit. The blocker from my prior review is addressed.
The missing chain-derived deps are gone, pnpm --filter app lint no longer reports the multi-chain hook dependency warnings, and the chain-change guard resets an active create flow before stale approve or lock values can be reused. The localStorage preset label key is also namespaced by chain ID now.
I do not see remaining blockers in this layer.
…d labels Addresses review feedback on #11. 1. Missing useEffect/useCallback deps for chain-derived values are now listed: usdcDecimals, brokerFee, sablierLockup, usdcAddress, treasury, explorerUrl. Without these, a chain switch mid-render could leave callbacks closed over stale values from the previous chain — most dangerously the approve→lock priming timeout, which would have fired writeLock against the old chain's Sablier address. 2. Chain-change guard: if useChainId() flips while step is past schedule (confirm/approve/lock), reset the form and toast the user. Stops the cross-chain race entirely. Uses a lastChainIdRef so the effect only acts on actual chain transitions, not initial mount. (The dep array includes step/resetForm/toast for lint, but the ref short-circuit means the body only runs when chainId changed.) 3. localStorage label key now namespaced by chainId: `ripguard:lock:<chainId>:<streamId>`. Sablier stream IDs reset per chain — without this, stream #5 on Arbitrum would steal the schedule label of stream #5 on Base. getLockLabel takes chainId as a parameter; VaultCard receives it as a prop from VaultDashboard. Existing locks under the old key fall back to the generic schedule type on /vaults — acceptable, since the fallback path was already in place. Refs: RG-9430aa (review feedback) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…#11) Switches /create, /vaults, and the landing page from default-chain singletons to getChainConfig(useChainId()) lookups. Every per-chain value — Sablier address, USDC address, USDC decimals, treasury, explorer URL, broker fee, stream start block, log chunk size — now flows from the active chain's registry entry. The minimum lock amount is now BigInt(10) ** BigInt(usdcDecimals) so BNB's 18-decimal USDC also enforces a 1-USDC floor. Sub-components (VestingCalculator, TimelinePreview, ConfirmDialog, SuccessView, VaultCard) take chain-derived values as props. Includes the review fixes: chain-aware deps on every callback/effect, chain-change guard with lastChainIdRef so an active tx flow resets if the wallet switches networks mid-flight, localStorage label key namespaced by chainId so cross-chain stream IDs don't collide. Refactors BROKER_FEE singleton into BROKER_FEE_RATE + helper functions brokerFeeForTreasury(treasury) and brokerFeePctString(fee), so each chain's fee is computed from its active treasury rather than the deployment default. Parameterizes parseAmountParam with a decimals cap (default 6, BNB needs 18) so URL-param sanity regex doesn't truncate high-decimal amounts. Refs: RG-9430aa, RG-42e939 Closes #11 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Merged via 85b386d on main — the squash of #8 with --delete-branch caused this PR to be auto-closed before its base could flip to main, and GitHub then refused to reopen it ("Could not open the pull request"). Fix landed via local squash-merge of the rebased branch directly to main; same content, just bypassed the broken GitHub state. Lesson for future stacks: when squash-merging the base of a stack, don't pass --delete-branch. Let the next PR's base auto-flip to main, then delete the branch after. |
1 participant
Summary
Stacked on #8. Switches every consumer from the default-chain singletons to `getChainConfig(useChainId())` lookups, threads `usdcDecimals` through the approve/lock/share flow, and addresses the broker-fee follow-up from the #8 review.
Closes `RG-9430aa` (page refactor) and `RG-42e939` (per-chain decimals).
What changed
`config/contracts.ts` — chain-specific singletons (`SABLIER_LOCKUP`, `USDC_ADDRESS`, `TREASURY`, `EXPLORER_URL`, `STREAM_START_BLOCK`, `LOG_CHUNK_SIZE`) deleted. `BROKER_FEE` becomes `BROKER_FEE_RATE` (the constant rate) plus two helpers — `brokerFeeForTreasury(treasury)` and `brokerFeePctString(fee)` — so each call site computes broker fee from its active chain's treasury (per the #8 review note). `IS_TESTNET` (site-level) and `PRESETS` (chain-agnostic) stay.
`lib/schedule.ts` — `parseAmountParam` now takes an optional decimal cap (default 6). BNB's 18-decimal USDC won't be truncated by the URL-param sanity regex.
`app/create/page.tsx` — `useChainId()` + destructured chain config at the top of `CreateLockInner`. `parseStreamIdFromReceipt` takes `sablierAddress` as a parameter. Sub-components (`VestingCalculator`, `TimelinePreview`, `ConfirmDialog`, `SuccessView`) take `usdcDecimals` / `brokerFee` / `brokerFeePct` / `sablierAddress` / `explorerUrl` as props.
`app/vaults/page.tsx` — `fetchFromSubgraph` and `fetchFromChain` take the active `chainId` and chain config as parameters. The Envio indexer query filters `chainId: { _eq: "" }` instead of the env-derived constant. `VaultCard` takes the chain-derived values as props.
`app/page.tsx` — landing reads the active chain via `useChainId()`. Unconnected users see the wagmi default chain's contract (Base on the mainnet site). The chain chip row in `RG-213ce7` will give per-chain links.
The minimum lock amount is now `BigInt(10) ** BigInt(usdcDecimals)` so BNB also enforces a 1-USDC floor.
Why a sub-component prop pattern instead of context
A React context for `useChainConfig()` would be cleaner long-term, but it adds a provider, opens up a wider blast radius, and isn't needed yet — only `/create` and `/vaults` have the sub-component depth that benefits from threaded values, and they're the only chain-aware pages. If the chip row in PR 4 ends up needing the chain config from many components, a context will make sense; for this PR, props keep the change focused.
Test plan
🤖 Generated with Claude Code