YNU-774: Add Native Tokens Support: Contract, SDK, Cerebro Updates#587
YNU-774: Add Native Tokens Support: Contract, SDK, Cerebro Updates#587
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds native-ETH and ERC-20 token operations across CLI, SDKs, blockchain clients, and contracts: new token-balance and approve CLI commands, on-chain balance and approve methods, native-token handling in EVM clients, a native Ether asset entry, and smart-contract decimals validation for native tokens. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (Operator)
participant SDK as SDK Client
participant Core as Go Core Client
participant EVM as EVM Blockchain Client
participant Node as Ethereum Node / Contract
rect rgba(200,200,255,0.5)
CLI->>SDK: approve(chainId, asset, amount)
end
rect rgba(200,255,200,0.5)
SDK->>Core: ApproveToken(ctx, chainId, asset, amount)
Core->>EVM: Approve(asset, amount)
end
rect rgba(255,230,200,0.5)
EVM->>Node: if token == zeroAddress ? (no-op / error) : send ERC20 approve tx
Node-->>EVM: txHash / error
EVM-->>Core: txHash / error
Core-->>SDK: txHash / error
SDK-->>CLI: display result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
Summary of ChangesHello @dimast-x, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the application by adding support for native tokens, alongside existing ERC-20 token functionality. This includes updates to the Cerebro CLI, SDK, and smart contracts to accommodate native token operations such as balance checks, approvals, and deposits. The changes ensure that the system can seamlessly handle both types of tokens, improving user experience and expanding the range of supported assets. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
0b2ec72 to
8db03d0
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for native tokens across the smart contracts, Go and TypeScript SDKs, and the Cerebro CLI. The changes are well-structured and effectively implement the required functionality. My review focuses on a couple of areas for improvement. In the Go EVM client, new functions use context.Background() instead of propagating a context from the caller, which I've recommended fixing. Additionally, in the TypeScript example application, the constant used for maximum token approval might be too low for its purpose, and I've suggested a more robust value. Overall, these are solid changes that accomplish the goal of adding native token support.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/core/interface.go (1)
18-19: AlignGetTokenBalanceparameter naming with asset-based semantics.Line 18 still uses
token, while the new flow is asset-driven. Renaming toassetwill reduce caller confusion and API misuse.♻️ Proposed naming-only diff
- GetTokenBalance(token string, account string) (decimal.Decimal, error) + GetTokenBalance(asset string, account string) (decimal.Decimal, error)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/core/interface.go` around lines 18 - 19, Rename the first parameter of the GetTokenBalance method from token to asset in the GetTokenBalance(token string, account string) (decimal.Decimal, error) signature to match the asset-driven flow; update all implementations of GetTokenBalance, any interface assertions, callers, tests, and imports that reference the old parameter name so the code compiles (ensure function/method names remain unchanged and only the parameter identifier is updated).sdk/ts/examples/example-app/src/components/ActionModal.tsx (1)
9-9: CentralizeMAX_APPROVE_AMOUNTto avoid cross-component drift.This constant is now duplicated here and in
sdk/ts/examples/example-app/src/components/WalletDashboard.tsx. A shared constant module would keep approval behavior consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts/examples/example-app/src/components/ActionModal.tsx` at line 9, MAX_APPROVE_AMOUNT is duplicated across components (ActionModal and WalletDashboard); extract it into a shared exported constant (e.g., export const MAX_APPROVE_AMOUNT = new Decimal('1e18')) in a new module (suggest name: constants or shared/constants) and update both ActionModal (function/component ActionModal) and WalletDashboard (component WalletDashboard) to import that constant instead of redefining it; ensure the new module also exports any required Decimal import or that each file still imports Decimal only if needed for other logic.sdk/ts/src/blockchain/evm/client.ts (1)
100-103: Prefer explicit native allowance bypass over a magic allowance value.Line 102 returns a synthetic
1e18allowance for native assets. It works in practice, but it encodes “not applicable” as a fake number. Consider bypassing allowance checks explicitly for native tokens in deposit-intent paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts/src/blockchain/evm/client.ts` around lines 100 - 103, Replace the synthetic magic allowance value by explicitly signaling “not applicable” for native tokens: in the function that checks allowance (the branch that currently uses tokenAddress and zeroAddress), return a clearly typed sentinel (e.g., null or undefined or a dedicated enum/value) instead of new Decimal('1e18') and update calling code in deposit/intent flows to treat that sentinel as “bypass allowance checks” (or add an isNative boolean derived from tokenAddress === zeroAddress to skip allowance logic). Reference tokenAddress and zeroAddress to locate the branch and ensure all callers that expect a Decimal handle the new sentinel or isNative flag accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/blockchain/evm/client.go`:
- Around line 281-284: The code mutates the shared client field
c.transactOpts.Value (seen in the block and also in Checkpoint and Create) which
risks races; instead make a per-call copy of c.transactOpts (e.g., local :=
*c.transactOpts), set local.Value = amountBig (or nil) as needed, and use that
local copy when invoking contract transactor methods so the client-level
transactOpts is never mutated concurrently; apply the same change to the
Checkpoint and Create call sites to avoid the defer-reset pattern.
In `@sdk/go/channel.go`:
- Around line 991-1001: Update the doc comment for GetOnChainBalance so it no
longer says "ERC20 token balance" — make it clear the function returns on-chain
asset balances for both ERC20 tokens and native chain assets; keep the param
descriptions (ctx, chainID, asset, wallet) but note that asset may refer to
native assets as well, and update the return description to state the balance is
returned as a decimal adjusted for the asset's decimals (ERC20 or native).
Ensure this change is made in the comment block above the GetOnChainBalance
function.
---
Nitpick comments:
In `@pkg/core/interface.go`:
- Around line 18-19: Rename the first parameter of the GetTokenBalance method
from token to asset in the GetTokenBalance(token string, account string)
(decimal.Decimal, error) signature to match the asset-driven flow; update all
implementations of GetTokenBalance, any interface assertions, callers, tests,
and imports that reference the old parameter name so the code compiles (ensure
function/method names remain unchanged and only the parameter identifier is
updated).
In `@sdk/ts/examples/example-app/src/components/ActionModal.tsx`:
- Line 9: MAX_APPROVE_AMOUNT is duplicated across components (ActionModal and
WalletDashboard); extract it into a shared exported constant (e.g., export const
MAX_APPROVE_AMOUNT = new Decimal('1e18')) in a new module (suggest name:
constants or shared/constants) and update both ActionModal (function/component
ActionModal) and WalletDashboard (component WalletDashboard) to import that
constant instead of redefining it; ensure the new module also exports any
required Decimal import or that each file still imports Decimal only if needed
for other logic.
In `@sdk/ts/src/blockchain/evm/client.ts`:
- Around line 100-103: Replace the synthetic magic allowance value by explicitly
signaling “not applicable” for native tokens: in the function that checks
allowance (the branch that currently uses tokenAddress and zeroAddress), return
a clearly typed sentinel (e.g., null or undefined or a dedicated enum/value)
instead of new Decimal('1e18') and update calling code in deposit/intent flows
to treat that sentinel as “bypass allowance checks” (or add an isNative boolean
derived from tokenAddress === zeroAddress to skip allowance logic). Reference
tokenAddress and zeroAddress to locate the branch and ensure all callers that
expect a Decimal handle the new sentinel or isNative flag accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cerebro/commands.gocerebro/operator.goclearnode/chart/config/v1-rc/assets.yamlcontracts/src/Utils.solpkg/blockchain/evm/client.gopkg/core/interface.gosdk/go/channel.gosdk/ts/examples/example-app/src/components/ActionModal.tsxsdk/ts/examples/example-app/src/components/WalletDashboard.tsxsdk/ts/src/blockchain/evm/client.tssdk/ts/src/client.ts
04ab272 to
bee2ed1
Compare
Summary by CodeRabbit
New Features
User-facing Changes
Bug Fixes