Feat/helios light client verification#792
Open
pheobeayo wants to merge 4 commits intoenkryptcom:mainfrom
Open
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 35 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughIntroduces experimental Helios light-client-based ERC-20 balance verification for Ethereum mainnet. Adds a new verification module that initializes a Helios provider, performs balance verification through eth_call, and surfaces verification results (confirmed or tampering detected) through UI warnings and polling mechanisms integrated into the network assets view. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant AssetsView as Assets View
participant Erc20Token as Erc20Token
participant EvmNetwork as EvmNetwork
participant HeliosVerifier as Helios Verifier
participant Helios as Helios Client
participant RPC as Execution RPC
User->>AssetsView: View network assets
AssetsView->>EvmNetwork: getAllTokenInfo()
EvmNetwork->>Erc20Token: getLatestUserBalance(api, address, chainId)
Erc20Token->>RPC: Fetch balance via RPC
RPC-->>Erc20Token: rpcBalance
Erc20Token->>HeliosVerifier: verifyErc20Balance(...)
alt Mainnet + Helios not synced
HeliosVerifier->>HeliosVerifier: initHelios()
HeliosVerifier->>RPC: Fetch consensus checkpoint
RPC-->>HeliosVerifier: checkpoint
HeliosVerifier->>Helios: Initialize with checkpoint
Helios->>Helios: Sync headers
Helios-->>HeliosVerifier: Sync complete
end
alt Helios synced
HeliosVerifier->>Helios: eth_call balanceOf
Helios-->>HeliosVerifier: heliosBalance
HeliosVerifier->>HeliosVerifier: Compare balances
alt Balances match
HeliosVerifier-->>Erc20Token: {verified: true}
else Balances differ
HeliosVerifier-->>Erc20Token: {tampered: true, provenBalance}
end
else Not synced or not mainnet
HeliosVerifier-->>Erc20Token: {verified: false, message: "SKIP"}
end
Erc20Token-->>EvmNetwork: Balance + VerificationResult
EvmNetwork-->>AssetsView: Assets with heliosVerification
AssetsView->>AssetsView: scheduleHeliosResultCheck()
loop Poll (15×, 2s intervals)
AssetsView->>AssetsView: Check for tampered assets
alt Tampering detected
AssetsView->>AssetsView: Set heliosWarning
AssetsView->>User: Display warning banner
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes span multiple architectural layers with new async initialization logic, state management across modules, polling mechanisms, and UI integration. While individual cohorts are straightforward, the cross-module dependencies and verification flow require careful validation of initialization sequencing, error handling, and state consistency. Possibly related PRs
Suggested reviewers
🚥 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/extension/src/providers/ethereum/types/evm-network.ts (2)
165-180:⚠️ Potential issue | 🟠 MajorDon't snapshot
heliosVerificationbefore the async verifier finishes.At Lines 179 and 260 you're copying
token.heliosVerificationinto plainAssetsTypeobjects immediately aftergetLatestUserBalance()returns. Butpackages/extension/src/providers/ethereum/types/erc20-token.tsstartsverifyErc20Balance()asynchronously and returns the balance first, so this field is usually stillundefinedhere and never updates afterward. The warning flow will therefore miss real mismatches.Also applies to: 243-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension/src/providers/ethereum/types/evm-network.ts` around lines 165 - 180, The code snapshots token.heliosVerification into AssetsType immediately after getLatestUserBalance resolves, but verifyErc20Balance (in erc20-token.ts) runs async and may complete after that snapshot, so heliosVerification stays undefined; fix by waiting for the verifier to finish before building assetInfos: update the flow around this.assets.map/getLatestUserBalance to also await the token verification (either by modifying getLatestUserBalance to await verifyErc20Balance or by collecting and awaiting the verifier promises—e.g., call/await token.verifyErc20Balance() or use a Promise.all on tokens.map(t => t.verifyErc20Balance()) after getLatestUserBalance) and only then read token.heliosVerification into the AssetsType objects (refer to getLatestUserBalance, verifyErc20Balance, heliosVerification, AssetsType, and the assets mapping).
113-121:⚠️ Potential issue | 🔴 Critical
heliosVerificationis not a valid property onErc20TokenOptions.Line 116 assigns
heliosVerificationto thebTokenOptionsobject, butErc20TokenOptionsinterface only includescontract(extendingBaseTokenOptionswithname,symbol,decimals). This causes a TypeScript excess property error. TheErc20Tokenconstructor does not accept this property from options anyway.Either add
heliosVerificationto theErc20TokenOptionsinterface and constructor, or remove it from the object literal at line 116.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension/src/providers/ethereum/types/evm-network.ts` around lines 113 - 121, The object literal bTokenOptions is assigning heliosVerification which is not part of the Erc20TokenOptions interface; remove the heliosVerification property from bTokenOptions (or alternatively, if you need that data, add heliosVerification to the Erc20TokenOptions interface and accept it in the Erc20Token constructor), then update any callers/constructors accordingly (reference: bTokenOptions, Erc20TokenOptions, Erc20Token constructor).
🧹 Nitpick comments (1)
packages/extension/src/providers/ethereum/types/erc20-token.ts.bak (1)
1-45: Remove the checked-in backup file.
erc20-token.ts.baklooks like an editor artifact rather than source. Keeping it beside the real implementation makes future edits and searches error-prone for no benefit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension/src/providers/ethereum/types/erc20-token.ts.bak` around lines 1 - 45, Delete the checked-in backup artifact containing the duplicate Erc20Token implementation (the file that defines class Erc20Token and interface Erc20TokenOptions) from the repository and staging area (use git rm --cached or git rm as appropriate) so it no longer appears beside the real source; ensure no code references rely on that backup, commit the removal, and add a rule (e.g., *.bak) to .gitignore to prevent future editor backup files from being committed.
🤖 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/extension/src/providers/ethereum/libs/helios-verifier.ts`:
- Around line 11-30: The current demo stub (heliosProvider.request, isSynced
default true, empty initHelios and resetHelios) must be replaced with real
Helios integration: implement initHelios(executionRpc, consensusRpc) to call
createHeliosProvider({executionRpc, consensusRpc}) to set heliosProvider, await
heliosProvider.waitSynced() and set isSynced accordingly (with timeout and error
handling), and recreate the provider if executionRpc/consensusRpc change;
implement resetHelios() to properly dispose/reset heliosProvider and set
isSynced = false; ensure heliosProvider.request delegates to the real provider
and surface errors rather than returning hardcoded values.
In `@packages/extension/src/providers/ethereum/types/erc20-token.ts`:
- Around line 47-58: The async verifyErc20Balance call can complete out-of-order
and overwrite this.heliosVerification for a newer request; to fix, add a
short-lived request identity check: when starting the verification (in the same
method that calls verifyErc20Balance), capture a unique token or incrementing
counter on the instance (e.g., this._heliosReqId or this._heliosPendingKey) and
close over it, then before assigning this.heliosVerification in the .then
handler compare the current instance token to the captured one and only assign
if they match; reference verifyErc20Balance, this.heliosVerification, address,
balance, chainId and api.node when locating the call site and add the token
property and comparison logic around the promise handlers.
In `@packages/extension/src/providers/ethereum/types/evm-network.ts.bak`:
- Around line 164-169: Calls to getLatestUserBalance are missing this.chainID
and the returned Helios verification is discarded, so token.heliosVerification
never propagates and tamper warnings can't fire; update each Promise.all/asset
mapping that calls token.getLatestUserBalance(api as API, address) (and similar
calls at the other mentioned blocks) to pass this.chainID (e.g.,
token.getLatestUserBalance(api as API, address, this.chainID)) and capture the
full result, then assign both token.balance and token.heliosVerification from
that result (e.g., const { balance, heliosVerification } = await
token.getLatestUserBalance(...); token.balance = balance;
token.heliosVerification = heliosVerification), and ensure any downstream asset
mappers do not overwrite or drop token.heliosVerification so the assets view can
read asset.heliosVerification?.tampered.
In `@packages/extension/src/ui/action/views/network-assets/index.vue`:
- Around line 78-98: scheduleHeliosResultCheck starts a new setInterval each
call but never clears any previous timer, causing stale polling to update
heliosWarning for old accounts/networks; change scheduleHeliosResultCheck to
store the interval id (e.g., in a module- or component-scoped variable or a ref
like heliosIntervalId), always clear any existing interval before creating a new
one, and clear that interval when attempts reach MAX_ATTEMPTS or when a tampered
result is found (already done) and also clear it on component teardown (use
onBeforeUnmount) and wherever updateAssets() or similar rescheduling logic runs;
apply the same fix to the other interval blocks (the similar polling code areas)
in this file so no orphaned timers remain.
In `@packages/extension/src/ui/action/views/network-assets/index.vue.bak`:
- Around line 129-148: The updateAssets function currently only snapshots
selectedNetworkName to guard against stale responses; capture the request
identity for account/subnetwork too (e.g., snapshot
props.accountInfo.selectedAccount?.address and any subnetwork id used) into
local constants (like currentNetwork and currentAddress/currentSubnetwork)
before calling props.network.getAllTokenInfo, and check those same constants
before mutating assets.value, isLoading.value, and isFetchError.value both in
the .then and .catch branches so responses from an earlier request cannot
overwrite newer data; update the guard logic that references selectedNetworkName
to also verify the saved address/subnetwork (or use a single requestId token)
when resolving or rejecting in updateAssets.
---
Outside diff comments:
In `@packages/extension/src/providers/ethereum/types/evm-network.ts`:
- Around line 165-180: The code snapshots token.heliosVerification into
AssetsType immediately after getLatestUserBalance resolves, but
verifyErc20Balance (in erc20-token.ts) runs async and may complete after that
snapshot, so heliosVerification stays undefined; fix by waiting for the verifier
to finish before building assetInfos: update the flow around
this.assets.map/getLatestUserBalance to also await the token verification
(either by modifying getLatestUserBalance to await verifyErc20Balance or by
collecting and awaiting the verifier promises—e.g., call/await
token.verifyErc20Balance() or use a Promise.all on tokens.map(t =>
t.verifyErc20Balance()) after getLatestUserBalance) and only then read
token.heliosVerification into the AssetsType objects (refer to
getLatestUserBalance, verifyErc20Balance, heliosVerification, AssetsType, and
the assets mapping).
- Around line 113-121: The object literal bTokenOptions is assigning
heliosVerification which is not part of the Erc20TokenOptions interface; remove
the heliosVerification property from bTokenOptions (or alternatively, if you
need that data, add heliosVerification to the Erc20TokenOptions interface and
accept it in the Erc20Token constructor), then update any callers/constructors
accordingly (reference: bTokenOptions, Erc20TokenOptions, Erc20Token
constructor).
---
Nitpick comments:
In `@packages/extension/src/providers/ethereum/types/erc20-token.ts.bak`:
- Around line 1-45: Delete the checked-in backup artifact containing the
duplicate Erc20Token implementation (the file that defines class Erc20Token and
interface Erc20TokenOptions) from the repository and staging area (use git rm
--cached or git rm as appropriate) so it no longer appears beside the real
source; ensure no code references rely on that backup, commit the removal, and
add a rule (e.g., *.bak) to .gitignore to prevent future editor backup files
from being committed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0c83e3f2-6c98-40da-b229-0b29f30626a3
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
packages/extension/package.jsonpackages/extension/src/providers/ethereum/libs/helios-verifier.tspackages/extension/src/providers/ethereum/types/erc20-token.tspackages/extension/src/providers/ethereum/types/erc20-token.ts.bakpackages/extension/src/providers/ethereum/types/evm-network.tspackages/extension/src/providers/ethereum/types/evm-network.ts.bakpackages/extension/src/types/provider.tspackages/extension/src/ui/action/icons/common/shield-alert-icon.vuepackages/extension/src/ui/action/views/network-assets/components/helios-warning-banner.vuepackages/extension/src/ui/action/views/network-assets/index.vuepackages/extension/src/ui/action/views/network-assets/index.vue.bak
packages/extension/src/providers/ethereum/libs/helios-verifier.ts
Outdated
Show resolved
Hide resolved
packages/extension/src/providers/ethereum/types/evm-network.ts.bak
Outdated
Show resolved
Hide resolved
packages/extension/src/ui/action/views/network-assets/index.vue.bak
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/extension/src/ui/action/views/network-assets/index.vue (1)
112-123:⚠️ Potential issue | 🟠 MajorClear the old Helios poll at the start of
updateAssets().
updateAssets()can run without the watcher (@update:tokens, retry, token-added flows). In those paths, the previous interval keeps running until the new request resolves and can repopulateheliosWarningfrom the stale snapshot during loading.Possible fix
const updateAssets = () => { + clearHeliosPoll(); isFetchError.value = false; isLoading.value = true; assets.value = []; heliosWarning.value = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension/src/ui/action/views/network-assets/index.vue` around lines 112 - 123, The updateAssets() function can leave a previous Helios polling interval running and repopulate heliosWarning from stale state; at the top of updateAssets() clear the existing Helios poll before mutating state by calling clearInterval/clearTimeout on the Helios poll handle (e.g., heliosPoll or heliosPollId) and reset that handle to null/undefined, then proceed to set isFetchError, isLoading, assets, heliosWarning and heliosBannerDismissed as currently done; ensure the code references the same poll handle used when starting the poll so no stale interval continues running.
🤖 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/extension/src/providers/ethereum/libs/helios-verifier.ts`:
- Around line 94-125: The verification currently calls eth_call with the
'latest' block which can differ from the block used to obtain rpcBalance,
causing false tamper reports; update verifyErc20Balance to accept the original
block tag/number (e.g., rpcBlockTag or rpcBlockNumber) alongside rpcBalance and
use that block tag in the heliosProvider.request params instead of 'latest' so
helios proves the balance at the same block as the RPC read; keep existing error
handling and returned fields (verified, tampered, message, provenBalance) but
ensure you pass the block tag through any callers that perform the initial RPC
balance read.
---
Duplicate comments:
In `@packages/extension/src/ui/action/views/network-assets/index.vue`:
- Around line 112-123: The updateAssets() function can leave a previous Helios
polling interval running and repopulate heliosWarning from stale state; at the
top of updateAssets() clear the existing Helios poll before mutating state by
calling clearInterval/clearTimeout on the Helios poll handle (e.g., heliosPoll
or heliosPollId) and reset that handle to null/undefined, then proceed to set
isFetchError, isLoading, assets, heliosWarning and heliosBannerDismissed as
currently done; ensure the code references the same poll handle used when
starting the poll so no stale interval continues running.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dcaea652-25c3-4ab4-b18e-b9e2ca6b9da8
📒 Files selected for processing (2)
packages/extension/src/providers/ethereum/libs/helios-verifier.tspackages/extension/src/ui/action/views/network-assets/index.vue
packages/extension/src/providers/ethereum/libs/helios-verifier.ts
Outdated
Show resolved
Hide resolved
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.
Implemented a feature for an experimental Helios light client integration for ERC-20 balance verification
Summary by CodeRabbit
New Features
Chores