refactor: migrate RevokeDelegationPanel to meteorology gas + parallel multi-chain revoke#7171
Open
DanielSinclair wants to merge 5 commits intodevelopfrom
Open
refactor: migrate RevokeDelegationPanel to meteorology gas + parallel multi-chain revoke#7171DanielSinclair wants to merge 5 commits intodevelopfrom
DanielSinclair wants to merge 5 commits intodevelopfrom
Conversation
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
DanielSinclair
changed the title
Contributor
Author
|
/e2e |
DanielSinclair
requested review from
christianbaroni,
ibrahimtaveras00,
janek26 and
janicduplessis
and removed request for
christianbaroni and
janicduplessis
DanielSinclair
force-pushed
the
@daniel/7702-revoke-gas
branch
from
6520921 to
1478907
Compare
DanielSinclair
force-pushed
the
@daniel/7702-revoke-gas
branch
from
1478907 to
de3d421
Compare
|
🧪 Flashlight Performance Report (AWS Device Farm) 🔀 Commit: d015096
|
…state management fix: useEffect revoke panel dismissal useMeteorologySuggestionMultichain fix: stableDelegationsToRevoke refactor: useRevokeDelegation Simplify setChainStatus typing Fix revoke preflight to read balances at action time
Add focused tests for partitionChainsByGasAvailability to lock in the new gas-eligibility behavior. The suite verifies: (1) cached zero native balance is insufficient without RPC fallback, (2) missing cache falls back to provider balance, and (3) balance lookup failures stay actionable to avoid false negatives.
Revoke preflight previously treated chains as actionable when the native asset cache entry was missing. On cold or stale caches this could skip insufficient-gas classification and proceed into avoidable revoke failures. This change keeps existing cached-balance behavior, but when native asset data is unavailable it now falls back to provider.getBalance(address) per chain before partitioning actionable vs insufficientGas chains. The hook now runs this partition asynchronously and passes address into the preflight helper, so behavior stays minimal and localized to gas eligibility checks.
DanielSinclair
force-pushed
the
@daniel/7702-revoke-gas
branch
from
028ebcb to
a91caff
Compare
maxbbb
requested changes
Feb 27, 2026
Contributor
maxbbb
left a comment
maxbbb
left a comment
There was a problem hiding this comment.
Looks good, just a few small things / questions.
Comment on lines
+114
to
+116
| staleTime: staleTime ?? 12_000, // 12 seconds | ||
| cacheTime: cacheTime ?? 36_000, // 36 seconds | ||
| refetchInterval: refetchInterval ?? 12_000, // 12 seconds |
Contributor
There was a problem hiding this comment.
super nit: would pull these into constants at top of function defined with time util from utils/time since they're shared between this and the regular useMeteorology
Comment on lines
+24
to
+62
| export async function partitionChainsByGasAvailability({ | ||
| chains, | ||
| address, | ||
| getNativeAssetBalance, | ||
| getChainBalance, | ||
| }: PartitionGasChainsArgs): Promise<{ insufficientGasChainIds: number[]; actionableChains: ChainWithId[] }> { | ||
| const insufficientGasChainIds: number[] = []; | ||
| const actionableChains: ChainWithId[] = []; | ||
|
|
||
| for (const d of chains) { | ||
| const chainId = d.chainId as ChainId; | ||
| const nativeAssetBalance = getNativeAssetBalance(chainId); | ||
|
|
||
| if (nativeAssetBalance != null && Number(nativeAssetBalance) <= 0) { | ||
| insufficientGasChainIds.push(d.chainId); | ||
| continue; | ||
| } | ||
| if (nativeAssetBalance != null && Number(nativeAssetBalance) > 0) { | ||
| actionableChains.push(d); | ||
| continue; | ||
| } | ||
|
|
||
| if (!address) { | ||
| actionableChains.push(d); | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| const onchainBalance = await getChainBalance(chainId, address); | ||
| if (onchainBalance <= 0n) insufficientGasChainIds.push(d.chainId); | ||
| else actionableChains.push(d); | ||
| } catch { | ||
| // If balance fetch fails, keep chain actionable and let execution return the real failure reason. | ||
| actionableChains.push(d); | ||
| } | ||
| } | ||
|
|
||
| return { insufficientGasChainIds, actionableChains }; | ||
| } |
Contributor
There was a problem hiding this comment.
a few nits:
- Only thing this effects is the fallback case, so not that consequential, but ideally this is done in a
Promise.allorPromise.allSettledso that you're not doing sequential fetches. - I'm not sure of other places in the app where we do a fallback direct chain lookup for balance, it might be harmless, but is it possible there are downstream consequences when this case is hit?
- Not sure there is a good reason to have one return type as
number[]and the otherChainWithId[], vs both just being one or the other.
Comment on lines
+105
to
+117
| setChainStatuses(prev => { | ||
| const next = { ...prev }; | ||
| for (const cid of chainIds) { | ||
| if (status === 'insufficientGas' && next[cid] === 'success') continue; | ||
| next[cid] = status as ChainRevokeStatus; | ||
| } | ||
| return next; | ||
| }); | ||
| }, []); | ||
|
|
||
| const mergeChainStatuses = useCallback((results: Record<number, ChainRevokeStatus>) => { | ||
| setChainStatuses(prev => ({ ...prev, ...results })); | ||
| }, []); |
Contributor
There was a problem hiding this comment.
These are doing basically the same thing, apart from the if (status === 'insufficientGas' && next[cid] === 'success') continue;, could they be combined?
|
|
||
| const handleRetry = useCallback(async () => { | ||
| const failedChains = delegationsToRevoke.filter(d => chainStatuses[d.chainId] === 'error'); | ||
| if (failedChains.length === 0) return; |
Contributor
There was a problem hiding this comment.
this condition is handled by revokeChains, do we need to also handle it here?
Comment on lines
+269
to
+275
| const onSuccessRef = useRef(onSuccess); | ||
| onSuccessRef.current = onSuccess; | ||
| useEffect(() => { | ||
| if (revokeStatus === 'success') { | ||
| onSuccessRef.current?.(); | ||
| } | ||
| }, [revokeStatus]); |
Contributor
There was a problem hiding this comment.
You could just use onSuccess directly and add // eslint-disable-next-line react-hooks/exhaustive-deps
| chainStatuses[cid] = 'error'; | ||
| anyFailed = true; | ||
| logger.error(new RainbowError('Failed to revoke delegation'), { | ||
| error: r.reason, |
Contributor
There was a problem hiding this comment.
r.reason should be passed as the second argument to RainbowError. Error in the object goes to Sentry as metadata and it often gets dropped, plus we lose the stack trace.
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
2 participants
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 changed
Splits
RevokeDelegationPanelinto a presentation layer + two hooks, and migrates gas from the legacyuseGaspolling hook to meteorology.RevokeDelegationPanel.tsxis now a pure presenter. All tx execution, state management, and gas logic moved out.useRevokeDelegation(new) handles the full lifecycle: per-chain status tracking, native-balance preflight (skips chains with no gas instead of failing), parallelPromise.allSettledexecution across all chains, single wallet load (one biometric prompt), and retry of only failed chains.useRevokeDelegationGas(new) estimates fees across all chains usinguseMeteorologySuggestionMultichain+estimateGas, sums them into one display value with the same gas buffer the delegation SDK applies.meteorology.tsaddsuseMeteorologyMultichain,useMeteorologySuggestionMultichain, andgetMeteorologyCachedDatafor multi-chain and non-React cache reads.types.ts(new) extractsRevokeReason,RevokeStatus, andChainRevokeStatusso they're importable without pulling in the panel.{ chainId: mainnet }fallbacks.Why
The old panel executed chains one at a time via component-level index state, prompted biometrics per chain, used the legacy gas hook, and had no concept of insufficient gas. This was slow for multi-chain revokes and had poor error UX.
Benefit
insufficientGasas a first-class state with clear messaging