YNU-308/318: streamline blockchains and assets configuration#397
YNU-308/318: streamline blockchains and assets configuration#397
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughCentralizes runtime configuration into YAML files for blockchains and assets, adds loaders/validators and tests, removes the DB Asset model in favor of config-driven assets, updates services and RPC types to consume the new config structures, and updates Helm, Docker Compose, integration tests, and CI to mount/load the config files. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as main.go
participant Config as config.go
participant BC as blockchain_config.go
participant AC as asset_config.go
participant Cust as custody.go
participant Channel as channel_service.go
Main->>Config: LoadConfig()
Config->>Config: load .env from CLEARNODE_CONFIG_DIR_PATH
Config->>BC: LoadBlockchains(configDir)
BC->>BC: parse YAML, verifyVariables()
BC->>BC: verifyRPCs (env RPC check, chainID)
BC-->>Config: map[chainID]BlockchainConfig
Config->>AC: LoadAssets(configDir)
AC->>AC: parse YAML, verifyVariables()
AC-->>Config: AssetsConfig
Config-->>Main: Config{blockchains, assets, ...}
Main->>Channel: NewChannelService(blockchains, &assets, signer)
Main->>Cust: NewCustody(blockchain, &assets, ...)
Cust->>Cust: use blockchain.ContractAddresses & assetsCfg for token lookups
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas to focus review on:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
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 @philanton, 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 significantly refactors the blockchain and asset configuration within the Clearnode application. It transitions from a verbose environment variable-driven setup to a more organized and maintainable YAML-based system. This change centralizes blockchain-specific settings, making it easier to define multiple networks, their contract addresses, and other parameters. The update also includes necessary adjustments to the application's core logic, RPC API, Helm charts, and Docker Compose setup to fully integrate and leverage the new configuration approach. Highlights
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
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Code Review
This pull request introduces a YAML-based configuration system for managing blockchain connections and contract addresses, replacing the previous method of relying on environment variables. It also refactors the code to use the new configuration system, improving maintainability and flexibility. The changes include adding a new blockchain_config.go file, modifying the README.md to document the new configuration, and updating the channel_service.go and config.go files to reflect the new configuration structure.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
clearnode/main.go (2)
88-95: Bug: custodyClients is shadowed, yielding an empty worker map.Inner custodyClients redeclaration hides the outer map; the loop iterates over the empty map.
- if len(custodyClients) > 0 { - custodyClients := make(map[uint32]CustodyInterface, len(custodyClients)) - for chainID, client := range custodyClients { - custodyClients[chainID] = client - } - worker := NewBlockchainWorker(db, custodyClients, logger) + if len(custodyClients) > 0 { + clientIfaces := make(map[uint32]CustodyInterface, len(custodyClients)) + for chainID, client := range custodyClients { + clientIfaces[chainID] = client + } + worker := NewBlockchainWorker(db, clientIfaces, logger) go worker.Start(context.Background()) }
121-125: Don't Fatal on normal RPC server shutdown.Handle http.ErrServerClosed like the metrics server to avoid spurious fatal logs during graceful shutdown.
- if err := rpcServer.ListenAndServe(); err != nil { - logger.Fatal("RPC server failure", "error", err) - } + if err := rpcServer.ListenAndServe(); err != nil && err != http.ErrServerClosed { + logger.Error("RPC server failure", "error", err) + }clearnode/channel_service.go (3)
63-70: Validate session key address; reject invalid hex.common.HexToAddress silently maps invalid input to 0x0. Validate before use.
userParticipant := wallet if params.SessionKey != nil { - sessionKeyAddress := common.HexToAddress(*params.SessionKey) + if !common.IsHexAddress(*params.SessionKey) { + return ChannelOperationResponse{}, RPCErrorf("invalid session key address") + } + sessionKeyAddress := common.HexToAddress(*params.SessionKey) if sessionKeyAddress == wallet { return ChannelOperationResponse{}, RPCErrorf("session key cannot be the same as the wallet address") } userParticipant = sessionKeyAddress }
146-156: Validate funds destination address in resize.Prevent zero-address allocations by verifying params.FundsDestination.
if params.AllocateAmount == nil { params.AllocateAmount = &decimal.Zero } + if !common.IsHexAddress(params.FundsDestination) { + return ChannelOperationResponse{}, RPCErrorf("invalid funds destination address") + } @@ allocations := []nitrolite.Allocation{ { - Destination: common.HexToAddress(params.FundsDestination), + Destination: common.HexToAddress(params.FundsDestination),(Validation added before allocations.)
Also applies to: 173-184
246-257: Validate funds destination address in close.Same risk in RequestClose.
asset, err := GetAssetByToken(s.db, channel.Token, channel.ChainID) @@ if balance.IsNegative() { logger.Error("negative balance", "balance", balance.String()) return ChannelOperationResponse{}, RPCErrorf("negative balance") } + if !common.IsHexAddress(params.FundsDestination) { + return ChannelOperationResponse{}, RPCErrorf("invalid funds destination address") + }Also applies to: 269-280
clearnode/pkg/rpc/api.go (1)
493-503: Unify BlockchainInfo and BrokerConfig types; eliminate duplicate definitions.Verification confirms identical
BlockchainInfostructs exist in bothclearnode/rpc_router_public.go(line 75) andclearnode/pkg/rpc/api.go(line 494). Additionally,BrokerConfigis duplicated across the same files with conflicting field names (NetworksvsBlockchains) but identical JSON keys ("networks"), creating maintenance burden and risk of divergence.Recommended action: Keep definitions in
clearnode/pkg/rpc/api.go(the canonical pkg location) and import them inclearnode/rpc_router_public.go. Align theBrokerConfigfield names or consolidate to a single definition.
🧹 Nitpick comments (14)
clearnode/chart/values.yaml (1)
34-35: Consider adding documentation for the blockchains configuration schema.While the comment is present, users would benefit from inline examples or a reference to the expected structure (e.g., referencing
chart/config/*/blockchains.yamlfiles).Example enhancement:
- # -- Blockchains configuration + # -- Blockchains configuration + # See chart/config/sandbox/blockchains.yaml for structure reference blockchains: {}clearnode/config/compose/example/.env (1)
1-3: Consider removing unnecessary quotes from environment variable values.Environment variable values typically don't require quotes in
.envfiles. Modern dotenv parsers handle unquoted values correctly.-BROKER_PRIVATE_KEY="0xac0974bec38a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80" -POLYGON_AMOY_BLOCKCHAIN_RPC="wss://polygon-amoy-bor-rpc.publicnode.com" -BASE_SEPOLIA_BLOCKCHAIN_RPC="wss://base-sepolia.drpc.org" +BROKER_PRIVATE_KEY=0xac0974bec38a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80 +POLYGON_AMOY_BLOCKCHAIN_RPC=wss://polygon-amoy-bor-rpc.publicnode.com +BASE_SEPOLIA_BLOCKCHAIN_RPC=wss://base-sepolia.drpc.orgclearnode/blockchain_config_test.go (2)
10-16: Make subtests parallel-safe and faster.Use t.Parallel() and rebind tc inside the loop to avoid races and speed up execution.
func TestBlockchainConfig_verifyVariables(t *testing.T) { + t.Parallel() tcs := []struct { @@ - for _, tc := range tcs { - t.Run(tc.name, func(t *testing.T) { + for _, tc := range tcs { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() err := tc.cfg.verifyVariables()Also applies to: 261-275
264-267: Reduce brittleness of error checks.Comparing exact error strings is fragile. Prefer assert.ErrorContains or typed/sentinel errors and errors.Is.
- require.Error(t, err) - assert.Equal(t, tc.expectedErrorStr, err.Error()) + require.Error(t, err) + assert.ErrorContains(t, err, tc.expectedErrorStr)clearnode/main.go (1)
76-84: Optional: wire cancellable contexts for goroutines.Pass a cancelable context to client.ListenEvents and worker.Start; cancel it on shutdown.
Can add:
- ctx, cancel := context.WithCancel(context.Background())
- defer cancel() just before shutdown; use ctx for goroutines.
clearnode/pkg/rpc/api.go (1)
485-491: Plan the JSON field rename with back-compat.Current tag keeps "networks". Either keep it or emit both "blockchains" and "networks" during a deprecation window.
Example (custom marshal, minimal risk):
type BrokerConfig struct { BrokerAddress string `json:"broker_address"` - Blockchains []BlockchainInfo `json:"networks"` // TODO: rename to "blockchains" + Blockchains []BlockchainInfo `json:"-"` } + +func (b BrokerConfig) MarshalJSON() ([]byte, error) { + type alias BrokerConfig + return json.Marshal(struct { + alias + Blockchains []BlockchainInfo `json:"blockchains"` + Networks []BlockchainInfo `json:"networks"` + }{ + alias: alias(b), + Blockchains: b.Blockchains, + Networks: b.Blockchains, + }) +}Update comments/docstrings accordingly.
clearnode/channel_service.go (2)
58-61: Rename variable for clarity after refactor.networkConfig no longer reflects the concept; use blockchainCfg.
- networkConfig, ok := s.blockchains[params.ChainID] + blockchainCfg, ok := s.blockchains[params.ChainID] @@ - Adjudicator: common.HexToAddress(networkConfig.ContractAddresses.Adjudicator), + Adjudicator: common.HexToAddress(blockchainCfg.ContractAddresses.Adjudicator),Also applies to: 72-77
143-148: Avoid taking address of decimal.Zero global.Use a local zero to avoid shared-pointer surprises.
- if params.ResizeAmount == nil { - params.ResizeAmount = &decimal.Zero - } - if params.AllocateAmount == nil { - params.AllocateAmount = &decimal.Zero - } + if params.ResizeAmount == nil { + z := decimal.Zero + params.ResizeAmount = &z + } + if params.AllocateAmount == nil { + z := decimal.Zero + params.AllocateAmount = &z + }clearnode/channel_service_test.go (2)
329-343: Add negative tests for invalid session key and destination addresses.Cover invalid hex inputs to ensure handlers reject them.
@@ t.Run("RequestCreate_ZeroAmount", func(t *testing.T) { @@ }) + +t.Run("RequestCreate_ErrorInvalidSessionKey", func(t *testing.T) { + db, cleanup := setupTestDB(t); t.Cleanup(cleanup) + asset := seedAsset(t, db, tokenAddress, chainID, tokenSymbol, 6) + service := NewChannelService(db, blockchains, &signer) + amount := decimal.NewFromInt(1) + bad := "not-an-address" + params := getCreateChannelParams(chainID, asset.Token, amount) + params.SessionKey = &bad + _, err := service.RequestCreate(userAddress, params, rpcSigners, LoggerFromContext(context.Background())) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid session key") +}) + +t.Run("RequestResize_ErrorInvalidDestination", func(t *testing.T) { + db, cleanup := setupTestDB(t); t.Cleanup(cleanup) + asset := seedAsset(t, db, tokenAddress, chainID, tokenSymbol, 6) + ch := seedChannel(t, db, channelID, userAddress.Hex(), userAddress.Hex(), asset.Token, chainID, channelAmountRaw, 1, ChannelStatusOpen) + service := NewChannelService(db, nil, &signer) + amt := decimal.NewFromInt(1) + _, err := service.RequestResize(getResizeChannelParams(ch.ChannelID, &amt, nil, "bad"), rpcSigners, LoggerFromContext(context.Background())) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid funds destination") +})
91-135: Enable subtest parallelization.Add t.Parallel() within each t.Run body to reduce runtime; test DB is isolated per subtest.
t.Run("RequestResize_Success", func(t *testing.T) { + t.Parallel() @@ t.Run("RequestResize_SuccessfulDeallocation", func(t *testing.T) { + t.Parallel() @@ t.Run("RequestResize_ErrorInvalidChannelID", func(t *testing.T) { + t.Parallel() @@ t.Run("RequestResize_ErrorChannelClosed", func(t *testing.T) { + t.Parallel() @@ t.Run("RequestResize_ErrorOtherChallengedChannel", func(t *testing.T) { + t.Parallel() @@ t.Run("RequestResize_ErrorInsufficientFunds", func(t *testing.T) { + t.Parallel() @@ t.Run("RequestResize_ErrorInvalidSignature", func(t *testing.T) { + t.Parallel() @@ t.Run("RequestClose_Success", func(t *testing.T) { + t.Parallel() @@ t.Run("RequestClose_ErrorOtherChallengedChannel", func(t *testing.T) { + t.Parallel() @@ t.Run("RequestCreate_Success", func(t *testing.T) { + t.Parallel() @@ t.Run("RequestCreate_ZeroAmount", func(t *testing.T) { + t.Parallel() @@ t.Run("RequestCreate_ErrorInvalidSignature", func(t *testing.T) { + t.Parallel() @@ t.Run("RequestCreate_ErrorExistingOpenChannel", func(t *testing.T) { + t.Parallel() @@ t.Run("RequestCreate_ErrorUnsupportedToken", func(t *testing.T) { + t.Parallel() @@ t.Run("RequestCreate_ErrorUnsupportedChainID", func(t *testing.T) { + t.Parallel() @@ t.Run("RequestCreate_LargeAmount", func(t *testing.T) { + t.Parallel() @@ t.Run("RequestCreate_ErrorDifferentUserSignature", func(t *testing.T) { + t.Parallel()Also applies to: 137-163, 165-176, 178-193, 194-209, 210-231, 232-249, 250-277, 278-292, 293-327, 329-343, 345-361, 362-377, 378-393, 394-409, 410-424, 425-451
clearnode/blockchain_config.go (1)
71-83: Prefer os.Open and enforce YAML known fieldsO_SYNC isn’t needed for reads. Also, enforce KnownFields to catch typos in YAML.
- f, err := os.OpenFile(blockchainsPath, os.O_RDONLY|os.O_SYNC, 0) + f, err := os.Open(blockchainsPath) if err != nil { return nil, err } defer f.Close() - var cfg BlockchainsConfig - if err := yaml.NewDecoder(f).Decode(&cfg); err != nil { + var cfg BlockchainsConfig + dec := yaml.NewDecoder(f) + dec.KnownFields(true) + if err := dec.Decode(&cfg); err != nil { return nil, err }clearnode/rpc_router_public_test.go (1)
69-81: Align tests with canonical rpc typesIf you switch to rpc.BrokerConfig/BlockchainInfo in the router, update the test cast/import accordingly.
@@ -import ( +import ( "context" "encoding/json" "testing" "time" + "github.com/erc7824/nitrolite/clearnode/pkg/rpc" @@ - configMap, ok := res.Params.(BrokerConfig) + configMap, ok := res.Params.(rpc.BrokerConfig) require.True(t, ok, "Response should contain a BrokerConfig")Also applies to: 53-57
clearnode/custody.go (2)
56-59: Add a timeout when querying ChainIDAvoid potentially hanging on slow RPCs during startup.
- chainID, err := client.ChainID(context.Background()) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + chainID, err := client.ChainID(ctx) if err != nil { return nil, fmt.Errorf("failed to get chain ID: %w", err) }
89-95: Safe uint32 cast for chain IDUse Uint64 and guard against overflow before storing as uint32.
+ // Ensure chain ID fits into uint32 for internal storage + cid64 := chainID.Uint64() + if cid64 > math.MaxUint32 { + return nil, fmt.Errorf("chain ID too large: %d", cid64) + } @@ - chainID: uint32(chainID.Int64()), + chainID: uint32(cid64), signer: signer, - adjudicatorAddress: common.HexToAddress(blockchain.ContractAddresses.Adjudicator), + adjudicatorAddress: common.HexToAddress(blockchain.ContractAddresses.Adjudicator), wsNotifier: wsNotifier, blockStep: blockchain.BlockStep, logger: logger.NewSystem("custody").With("chainID", chainID.Int64()).With("custodyAddress", blockchain.ContractAddresses.Custody),Remember to import math:
-import ( +import ( "context" "errors" "fmt" + "math" "math/big" "time"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
examples/cerebro/go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (30)
clearnode/.gitignore(1 hunks)clearnode/README.md(4 hunks)clearnode/blockchain_config.go(1 hunks)clearnode/blockchain_config_test.go(1 hunks)clearnode/channel_service.go(3 hunks)clearnode/channel_service_test.go(17 hunks)clearnode/chart/config/prod/blockchains.yaml(1 hunks)clearnode/chart/config/prod/clearnode.yaml(0 hunks)clearnode/chart/config/sandbox/blockchains.yaml(1 hunks)clearnode/chart/config/sandbox/clearnode.yaml(0 hunks)clearnode/chart/config/uat/blockchains.yaml(1 hunks)clearnode/chart/config/uat/clearnode.yaml(0 hunks)clearnode/chart/templates/configmap.yaml(1 hunks)clearnode/chart/templates/deployment.yaml(3 hunks)clearnode/chart/values.yaml(1 hunks)clearnode/config.go(4 hunks)clearnode/config/compose/example/.env(1 hunks)clearnode/config/compose/example/blockchains.yaml(1 hunks)clearnode/custody.go(3 hunks)clearnode/docker-compose.yml(2 hunks)clearnode/go.mod(1 hunks)clearnode/main.go(2 hunks)clearnode/pkg/rpc/api.go(1 hunks)clearnode/pkg/rpc/client_manual_test.go(1 hunks)clearnode/pkg/rpc/client_test.go(1 hunks)clearnode/reconcile_cli.go(4 hunks)clearnode/rpc_router_public.go(2 hunks)clearnode/rpc_router_public_test.go(2 hunks)clearnode/rpc_router_test.go(1 hunks)examples/cerebro/go.mod(2 hunks)
💤 Files with no reviewable changes (3)
- clearnode/chart/config/sandbox/clearnode.yaml
- clearnode/chart/config/prod/clearnode.yaml
- clearnode/chart/config/uat/clearnode.yaml
🧰 Additional context used
🧬 Code graph analysis (13)
clearnode/reconcile_cli.go (2)
sdk/src/abis/types.ts (1)
ContractAddresses(6-15)clearnode/custody.go (1)
Custody(34-47)
clearnode/blockchain_config_test.go (1)
clearnode/blockchain_config.go (3)
BlockchainsConfig(30-33)BlockchainConfig(37-51)ContractAddressesConfig(55-59)
clearnode/config.go (1)
clearnode/blockchain_config.go (2)
BlockchainConfig(37-51)LoadBlockchains(70-93)
clearnode/main.go (2)
clearnode/channel_service.go (1)
NewChannelService(23-25)clearnode/custody.go (1)
NewCustody(50-96)
clearnode/blockchain_config.go (2)
sdk/src/abis/types.ts (1)
ContractAddresses(6-15)clearnode/custody.go (1)
Custody(34-47)
clearnode/channel_service.go (2)
clearnode/blockchain_config.go (1)
BlockchainConfig(37-51)sdk/src/abis/types.ts (1)
ContractAddresses(6-15)
clearnode/pkg/rpc/client_test.go (2)
clearnode/pkg/rpc/api.go (1)
BlockchainInfo(494-503)clearnode/rpc_router_public.go (1)
BlockchainInfo(75-80)
clearnode/rpc_router_test.go (3)
clearnode/blockchain_config.go (2)
BlockchainConfig(37-51)ContractAddressesConfig(55-59)clearnode/custody.go (1)
Custody(34-47)clearnode/channel_service.go (1)
NewChannelService(23-25)
clearnode/pkg/rpc/api.go (1)
clearnode/rpc_router_public.go (1)
BlockchainInfo(75-80)
clearnode/custody.go (3)
clearnode/nitrolite/bindings.go (2)
NewCustody(147-153)Custody(88-92)clearnode/blockchain_config.go (1)
BlockchainConfig(37-51)clearnode/log.go (1)
Logger(12-35)
clearnode/channel_service_test.go (2)
clearnode/blockchain_config.go (2)
BlockchainConfig(37-51)ContractAddressesConfig(55-59)clearnode/channel_service.go (1)
NewChannelService(23-25)
clearnode/rpc_router_public.go (3)
clearnode/pkg/rpc/api.go (2)
BlockchainInfo(494-503)BrokerConfig(486-491)clearnode/config.go (1)
Config(18-23)sdk/src/abis/types.ts (1)
ContractAddresses(6-15)
clearnode/rpc_router_public_test.go (2)
clearnode/blockchain_config.go (2)
BlockchainConfig(37-51)ContractAddressesConfig(55-59)clearnode/custody.go (1)
Custody(34-47)
🪛 Checkov (3.2.334)
clearnode/docker-compose.yml
[medium] 40-41: Basic Auth Credentials
(CKV_SECRET_4)
🪛 dotenv-linter (4.0.0)
clearnode/config/compose/example/.env
[warning] 1-1: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 2-2: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 3-3: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 3-3: [UnorderedKey] The BASE_SEPOLIA_BLOCKCHAIN_RPC key should go before the BROKER_PRIVATE_KEY key
(UnorderedKey)
🪛 Gitleaks (8.28.0)
clearnode/config/compose/example/.env
[high] 1-1: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 YAMLlint (1.37.1)
clearnode/chart/templates/configmap.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (25)
examples/cerebro/go.mod (3)
1-55: Inconsistency between AI summary and actual code changes.The AI summary states that
gopkg.in/yaml.v3is now a direct dependency, but this dependency is not visible in the provided code snippet. Given the PR objective to introduce YAML-driven blockchain configuration, the absence of a YAML parser dependency in this module is unexpected.Please verify:
- Is
gopkg.in/yaml.v3or another YAML parser actually added as a direct requirement? (It may have been added but not shown in the provided snippet.)- Does the
examples/cerebromodule require YAML configuration support, or is this only used in other packages within the monorepo?If YAML support is needed, apply a diff like:
require ( github.com/c-bata/go-prompt v0.2.6 github.com/ethereum/go-ethereum v1.16.4 github.com/gorilla/websocket v1.5.3 github.com/jedib0t/go-pretty/v6 v6.6.7 github.com/shopspring/decimal v1.4.0 golang.org/x/term v0.35.0 gorm.io/driver/sqlite v1.6.0 gorm.io/gorm v1.31.0 + gopkg.in/yaml.v3 v3.x.x )
11-11: Addition of golang.org/x/term makes sense for the terminal UI.The direct dependency on
golang.org/x/term v0.35.0aligns with the example app's use ofgo-promptand terminal utilities for interactive CLI features.
50-54: Transitive dependencies verified as compatible.Running
go mod tidyandgo mod verifyconfirms the indirect golang.org/x dependencies are compatible with go-ethereum v1.16.4 and the broader dependency graph. Both commands completed successfully with no conflicts detected.clearnode/pkg/rpc/client_manual_test.go (1)
129-129: LGTM! Test updated to reflect new API structure.The change from
NetworkstoBlockchainscorrectly aligns with the broader refactoring to YAML-based blockchain configuration.clearnode/go.mod (1)
182-182: LGTM! Correct promotion of yaml.v3 to direct dependency.This change appropriately reflects the direct use of YAML parsing in the new blockchain configuration loader.
clearnode/.gitignore (1)
25-26: LGTM! Gitignore updated to reflect new configuration structure.The change from
.envtoconfig/compose/localaligns with the migration to YAML-based configuration and appropriately excludes local environment-specific settings.clearnode/config/compose/example/.env (1)
1-1: Example file uses well-known test key (expected).The private key is Hardhat's first default account, which is appropriate for an example configuration file in
config/compose/example/. The gitleaks warning is a false positive in this context.clearnode/chart/config/uat/blockchains.yaml (1)
1-15: LGTM! Well-structured blockchain configuration for UAT.The YAML structure is clean and consistent, with proper chain IDs (137 for Polygon, 11155111 for Ethereum Sepolia) and formatted contract addresses. The configuration aligns with the new centralized blockchain config model.
clearnode/chart/templates/configmap.yaml (1)
1-13: LGTM! Properly configured Helm hook for blockchain configuration.The ConfigMap is correctly set up with pre-install/pre-upgrade hooks, appropriate weight and deletion policy. The YAMLlint syntax error is a false positive—it doesn't recognize Helm's Go templating syntax.
clearnode/chart/templates/deployment.yaml (2)
22-22: Excellent addition of config checksum annotation.This ensures pods are automatically restarted when the blockchains configuration changes, preventing stale configurations.
35-36: LGTM! Complete and consistent ConfigMap integration.The environment variable, volume mount, and ConfigMap volume declaration are properly wired together:
BLOCKCHAINS_CONFIG_PATHpoints to the mounted location- Volume mount at
/app/configprovides the configuration directory- ConfigMap volume references the correct resource
All changes work cohesively to deliver the blockchain configuration to the application.
Also applies to: 49-58
clearnode/chart/config/sandbox/blockchains.yaml (2)
1-3: LGTM! Default contract addresses are well-structured.The default custody and adjudicator addresses follow the expected Ethereum address format and will be inherited by blockchains that don't specify overrides.
26-33: block_step: 499 is intentional configuration for XRPL EVM and related chains.Verification shows that
block_step: 499is consistently applied to specific blockchains across all configurations:
- Production:
world_chain,flow,xrpl_evm- Sandbox:
xrpl_evm_testnetThis is a deliberate pattern used in production environments, indicating the smaller value is necessary for these specific chain types (likely due to their block production characteristics or rate limiting requirements). No action required.
clearnode/README.md (4)
11-11: LGTM! Clear feature description.The updated feature description accurately reflects the YAML-based configuration system introduced in this PR.
132-163: LGTM! Comprehensive blockchain configuration documentation.The new Blockchain Configuration section clearly explains:
- YAML file structure and location
- Default contract addresses pattern
- Per-blockchain configuration options
- RPC endpoint setup via environment variables
The documentation aligns well with the implementation and provides helpful examples.
169-180: LGTM! Environment variables table is accurate and complete.The updated table correctly documents:
- The new
CONFIG_DIR_PATHvariable for locatingblockchains.yaml- The dynamic
<BLOCKCHAIN_NAME>_BLOCKCHAIN_RPCpattern for RPC endpoints- Consistent naming with
CLEARNODE_LOG_LEVELAll changes align with the new configuration system.
185-212: LGTM! Quick Start instructions are clear and accurate.The updated Quick Start section provides a straightforward workflow:
- Create config directory and copy example
blockchains.yaml- Configure environment variables including RPC endpoints
- Run with Docker Compose
The instructions align perfectly with the new YAML-based configuration system.
clearnode/config/compose/example/blockchains.yaml (1)
1-33: LGTM! Excellent example configuration.This example effectively demonstrates:
- Default contract addresses shared across blockchains
- Selective blockchain enablement (2 enabled, 3 disabled)
- Simple configuration (only
balance_checkeroverride)- Advanced configuration (full address overrides + custom
block_step)This serves as a comprehensive reference for users setting up their own configurations.
clearnode/chart/config/prod/blockchains.yaml (1)
1-45: LGTM! Production configuration is well-structured.The production configuration demonstrates:
- Seven mainnet blockchains with appropriate chain IDs
- Selective enablement (world_chain disabled, likely for phased rollout)
- Consistent use of
block_step: 499for specific chains (world_chain, flow, xrpl_evm)- Custom contract addresses for base network
The configuration follows the same structure as sandbox/example, making it easy to understand and maintain.
clearnode/pkg/rpc/client_test.go (1)
78-98: LGTM! Test correctly updated for API changes.The test has been properly updated to reflect the renaming:
Networks→BlockchainsNetworkInfo→BlockchainInfoChainID→IDThe test logic remains unchanged and continues to validate the broker configuration retrieval correctly.
clearnode/rpc_router_test.go (1)
114-135: LGTM! Test setup correctly migrated to BlockchainConfig.The test setup has been properly updated to:
- Use
map[uint32]BlockchainConfiginstead of the previous networks structure- Nest contract addresses in
ContractAddressesstruct withCustodyandAdjudicatorfields- Pass the blockchains map to
NewChannelServiceThe test data structure now matches the production configuration, improving test fidelity.
clearnode/reconcile_cli.go (1)
21-90: LGTM! Reconcile CLI correctly migrated to blockchain configuration.The CLI has been comprehensively updated:
- Variable naming improved (
chainIDinstead ofnetworkID)- Configuration lookup uses
config.blockchainsmap- Contract addresses accessed via
blockchain.ContractAddresses.CustodyNewCustodysignature simplified to acceptBlockchainConfigobject- All field references updated to match new structure
The migration is complete and consistent with the broader PR changes.
clearnode/config.go (3)
12-23: LGTM! Config structure updated for YAML-based configuration.The changes introduce:
configDirPathEnvconstant for locating the configuration directoryblockchainsfield replacing the previous networks structure- Clean, consistent naming with
CLEARNODE_prefixThe simplified structure makes configuration more maintainable.
29-39: LGTM! Config directory handling is well-implemented.The configuration loading now:
- Resolves config directory from
CLEARNODE_CONFIG_DIR_PATHor defaults to current directory- Loads
.envfrom the config directory (not just cwd)- Maintains backwards compatibility with warning for missing
.envThis provides flexibility for different deployment scenarios while maintaining sensible defaults.
41-90: LGTM! Config loading correctly integrated with blockchain configuration.The configuration loading flow now:
- Properly declares
errvariable before parsing database connection string- Calls
LoadBlockchains(configDirPath)to parse and validateblockchains.yaml- Handles errors appropriately with fatal logging for missing/invalid blockchain config
- Constructs the final
Configwith theblockchainsmapThe refactoring successfully replaces environment-variable-based network discovery with YAML-driven configuration while maintaining proper error handling.
| for i, bc := range cfg.Blockchains { | ||
| if !bc.Enabled { | ||
| continue | ||
| } | ||
|
|
||
| if !blockchainNameRegex.MatchString(bc.Name) { | ||
| return fmt.Errorf("invalid blockchain name '%s'", bc.Name) | ||
| } | ||
|
|
||
| if bc.ContractAddresses.Custody == "" { | ||
| if defaults.Custody == "" { | ||
| return fmt.Errorf("missing custody contract address for blockchain '%s'", bc.Name) | ||
| } else { | ||
| cfg.Blockchains[i].ContractAddresses.Custody = defaults.Custody | ||
| } | ||
| } else if !contractAddressRegex.MatchString(bc.ContractAddresses.Custody) { | ||
| return fmt.Errorf("invalid custody contract address '%s' for blockchain '%s'", bc.ContractAddresses.Custody, bc.Name) | ||
| } | ||
|
|
||
| if bc.ContractAddresses.Adjudicator == "" { | ||
| if defaults.Adjudicator == "" { | ||
| return fmt.Errorf("missing adjudicator contract address for blockchain '%s'", bc.Name) | ||
| } else { | ||
| cfg.Blockchains[i].ContractAddresses.Adjudicator = defaults.Adjudicator | ||
| } | ||
| } else if !contractAddressRegex.MatchString(bc.ContractAddresses.Adjudicator) { | ||
| return fmt.Errorf("invalid adjudicator contract address '%s' for blockchain '%s'", bc.ContractAddresses.Adjudicator, bc.Name) | ||
| } | ||
|
|
||
| if bc.ContractAddresses.BalanceChecker == "" { | ||
| if defaults.BalanceChecker == "" { | ||
| return fmt.Errorf("missing balance checker contract address for blockchain '%s'", bc.Name) | ||
| } else { | ||
| cfg.Blockchains[i].ContractAddresses.BalanceChecker = defaults.BalanceChecker | ||
| } | ||
| } else if !contractAddressRegex.MatchString(bc.ContractAddresses.BalanceChecker) { | ||
| return fmt.Errorf("invalid balance checker contract address '%s' for blockchain '%s'", bc.ContractAddresses.BalanceChecker, bc.Name) | ||
| } | ||
|
|
||
| if bc.BlockStep == 0 { | ||
| cfg.Blockchains[i].BlockStep = defaultBlockStep | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate duplicates and zero chain IDs in enabled blockchains
Guard against conflicting configs and accidental zero IDs.
func (cfg *BlockchainsConfig) verifyVariables() error {
defaults := cfg.DefaultContractAddresses
@@
- for i, bc := range cfg.Blockchains {
+ seenIDs := make(map[uint32]struct{})
+ seenNames := make(map[string]struct{})
+ for i, bc := range cfg.Blockchains {
if !bc.Enabled {
continue
}
@@
if !blockchainNameRegex.MatchString(bc.Name) {
return fmt.Errorf("invalid blockchain name '%s'", bc.Name)
}
+
+ if bc.ID == 0 {
+ return fmt.Errorf("invalid chain id 0 for blockchain '%s'", bc.Name)
+ }
+ if _, dup := seenIDs[bc.ID]; dup {
+ return fmt.Errorf("duplicate chain id '%d' across enabled blockchains", bc.ID)
+ }
+ seenIDs[bc.ID] = struct{}{}
+ if _, dup := seenNames[bc.Name]; dup {
+ return fmt.Errorf("duplicate blockchain name '%s' across enabled blockchains", bc.Name)
+ }
+ seenNames[bc.Name] = struct{}{}🤖 Prompt for AI Agents
In clearnode/blockchain_config.go around lines 110 to 152, add validation to
guard against enabled blockchains having a zero ChainID or duplicate ChainIDs:
while iterating enabled blockchains, track seen chain IDs in a map[uint64]bool
(or map[int64]bool depending on type), and if bc.ChainID == 0 return an error
like "chain ID for blockchain '%s' is zero", and if the ChainID is already in
the map return an error like "duplicate chain ID %d for blockchain '%s'". Only
consider blockchains where bc.Enabled == true for these checks and mark the
ChainID in the map after it passes validation.
There was a problem hiding this comment.
@nksazonov, thank you for the feedback! I'm glad the suggestion is helpful. Let me know if you'd like any assistance with implementing the validation or have any other questions! 😊
There was a problem hiding this comment.
For 0 chainID I'm not sure, for duplicates we can consider addition, but I still don't see it that necessary
nksazonov
left a comment
There was a problem hiding this comment.
Generally LGTM
Please, address minor mine and AI comments.
| for i, bc := range cfg.Blockchains { | ||
| if !bc.Enabled { | ||
| continue | ||
| } | ||
|
|
||
| if !blockchainNameRegex.MatchString(bc.Name) { | ||
| return fmt.Errorf("invalid blockchain name '%s'", bc.Name) | ||
| } | ||
|
|
||
| if bc.ContractAddresses.Custody == "" { | ||
| if defaults.Custody == "" { | ||
| return fmt.Errorf("missing custody contract address for blockchain '%s'", bc.Name) | ||
| } else { | ||
| cfg.Blockchains[i].ContractAddresses.Custody = defaults.Custody | ||
| } | ||
| } else if !contractAddressRegex.MatchString(bc.ContractAddresses.Custody) { | ||
| return fmt.Errorf("invalid custody contract address '%s' for blockchain '%s'", bc.ContractAddresses.Custody, bc.Name) | ||
| } | ||
|
|
||
| if bc.ContractAddresses.Adjudicator == "" { | ||
| if defaults.Adjudicator == "" { | ||
| return fmt.Errorf("missing adjudicator contract address for blockchain '%s'", bc.Name) | ||
| } else { | ||
| cfg.Blockchains[i].ContractAddresses.Adjudicator = defaults.Adjudicator | ||
| } | ||
| } else if !contractAddressRegex.MatchString(bc.ContractAddresses.Adjudicator) { | ||
| return fmt.Errorf("invalid adjudicator contract address '%s' for blockchain '%s'", bc.ContractAddresses.Adjudicator, bc.Name) | ||
| } | ||
|
|
||
| if bc.ContractAddresses.BalanceChecker == "" { | ||
| if defaults.BalanceChecker == "" { | ||
| return fmt.Errorf("missing balance checker contract address for blockchain '%s'", bc.Name) | ||
| } else { | ||
| cfg.Blockchains[i].ContractAddresses.BalanceChecker = defaults.BalanceChecker | ||
| } | ||
| } else if !contractAddressRegex.MatchString(bc.ContractAddresses.BalanceChecker) { | ||
| return fmt.Errorf("invalid balance checker contract address '%s' for blockchain '%s'", bc.ContractAddresses.BalanceChecker, bc.Name) | ||
| } | ||
|
|
||
| if bc.BlockStep == 0 { | ||
| cfg.Blockchains[i].BlockStep = defaultBlockStep | ||
| } | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clearnode/custody.go (1)
57-65: Avoid hard-coded GasPrice/GasLimit and shared TransactOpts mutation
- Static GasPrice/GasLimit can misprice on EIP-1559 chains.
- You later mutate c.transactOpts in Checkpoint; sharing a single TransactOpts across calls risks data races under concurrency.
Clone opts per tx and prefer EIP‑1559 fields (GasFeeCap/GasTipCap) or leave unset to let the backend fill.
- // TODO: estimate on the go. - auth.GasPrice = big.NewInt(30000000000) // 30 gwei. - auth.GasLimit = uint64(3000000) + // Let backend/do per-tx estimation; avoid static gas in shared opts. + // Leave GasPrice/GasFeeCap/GasTipCap unset here; set per tx as needed.Outside this hunk, adjust Checkpoint to clone:
// helper func cloneTxOpts(src *bind.TransactOpts) *bind.TransactOpts { dst := *src // shallow copy is enough for go-eth return &dst } // in Checkpoint: opts := cloneTxOpts(c.transactOpts) gp, _ := c.client.SuggestGasPrice(ctx) opts.GasPrice = new(big.Int).Mul(gp, big.NewInt(2)) tx, err := c.custody.Checkpoint(opts, channelID, nitroState, proofs)
♻️ Duplicate comments (5)
clearnode/blockchain_config.go (4)
167-176: Trim RPC env var and include endpoint in error for faster debuggingWhitespace in env can cause false negatives; and printing the endpoint helps triage.
- blockchainRPC := os.Getenv(fmt.Sprintf("%s_BLOCKCHAIN_RPC", strings.ToUpper(bc.Name))) + key := fmt.Sprintf("%s_BLOCKCHAIN_RPC", strings.ToUpper(bc.Name)) + blockchainRPC := strings.TrimSpace(os.Getenv(key)) if blockchainRPC == "" { - return fmt.Errorf("missing blockchain RPC for blockchain '%s'", bc.Name) + return fmt.Errorf("missing blockchain RPC for blockchain '%s' (env %s)", bc.Name, key) } @@ - if err := checkChainId(blockchainRPC, bc.ID); err != nil { - return fmt.Errorf("blockchain '%s' ChainID check failed: %w", bc.Name, err) + if err := checkChainId(blockchainRPC, bc.ID); err != nil { + return fmt.Errorf("blockchain '%s' ChainID check failed for RPC '%s': %w", bc.Name, blockchainRPC, err) }
22-25: Document regex intent (name and address format)Add short comments so future edits keep the intended constraints clear.
var ( - blockchainNameRegex = regexp.MustCompile(`^[a-z][a-z_]+[a-z]$`) - contractAddressRegex = regexp.MustCompile(`^0x[0-9a-fA-F]{40}$`) + // snake_case, starts/ends with a letter + blockchainNameRegex = regexp.MustCompile(`^[a-z][a-z_]+[a-z]$`) + // EVM address: 0x + 40 hex chars (case-insensitive) + contractAddressRegex = regexp.MustCompile(`^0x[0-9a-fA-F]{40}$`) )
201-206: LGTM: client is closed to prevent leaksThanks for adding defer client.Close(); this avoids socket/resource leaks.
110-152: Validate zero and duplicate chain IDs and duplicate names (enabled blockchains)Currently duplicates silently overwrite in getEnabled(); add checks during validation.
func (cfg *BlockchainsConfig) verifyVariables() error { @@ - for i, bc := range cfg.Blockchains { + seenIDs := make(map[uint32]struct{}) + seenNames := make(map[string]struct{}) + for i, bc := range cfg.Blockchains { if !bc.Enabled { continue } @@ - if !blockchainNameRegex.MatchString(bc.Name) { + if !blockchainNameRegex.MatchString(bc.Name) { return fmt.Errorf("invalid blockchain name '%s', should match snake_case format", bc.Name) } + + if bc.ID == 0 { + return fmt.Errorf("invalid chain id 0 for blockchain '%s'", bc.Name) + } + if _, ok := seenIDs[bc.ID]; ok { + return fmt.Errorf("duplicate chain id '%d' across enabled blockchains", bc.ID) + } + seenIDs[bc.ID] = struct{}{} + if _, ok := seenNames[bc.Name]; ok { + return fmt.Errorf("duplicate blockchain name '%s' across enabled blockchains", bc.Name) + } + seenNames[bc.Name] = struct{}{}Based on learnings
clearnode/rpc_router_public.go (1)
113-120: Good: canonical rpc types usedUsing rpc.BlockchainInfo and rpc.BrokerConfig prevents drift with public API.
Also applies to: 124-127
🧹 Nitpick comments (5)
clearnode/blockchain_config.go (2)
78-81: Harden YAML parsing: reject unknown fieldsEnable strict decoding to catch typos/misconfig early.
- if err := yaml.NewDecoder(f).Decode(&cfg); err != nil { + dec := yaml.NewDecoder(f) + dec.KnownFields(true) + if err := dec.Decode(&cfg); err != nil {
41-49: Recommendation is valid for future EVM compatibility, but optional for current codebase.EVM uses uint256 for chain IDs (EIP-1344), with EIP-2294 recommending a practical upper bound around uint64. Current configuration files show all active chain IDs well within uint32 range (largest is 1,449,000; uint32 max is 4,294,967,295), so no immediate risk exists.
However, widening from uint32 to uint64 would require coordinated changes across multiple locations:
BlockchainConfig.IDfield (lines 43)- Return type
map[uint32]BlockchainConfig→map[uint64]BlockchainConfig(line 188)- Function signatures throughout codebase (checkChainId, runChainWorker, CreateChannel, etc.)
- Test assertions expecting uint32
This is a reasonable forward-looking refactoring to align with EVM specifications and prevent future overflow issues when new chains are onboarded, but not urgently required given current usage patterns.
clearnode/custody.go (1)
50-51: Prefer DialContext with timeout for node connectivityAvoid indefinite dials; keep the client long-lived but bound the initial connect.
-func NewCustody(signer *Signer, db *gorm.DB, wsNotifier *WSNotifier, blockchain BlockchainConfig, logger Logger) (*Custody, error) { - client, err := ethclient.Dial(blockchain.BlockchainRPC) +func NewCustody(signer *Signer, db *gorm.DB, wsNotifier *WSNotifier, blockchain BlockchainConfig, logger Logger) (*Custody, error) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + client, err := ethclient.DialContext(ctx, blockchain.BlockchainRPC)clearnode/rpc_router_public.go (1)
113-127: Return blockchains in deterministic orderMap iteration is random; sort for stable API responses.
- supportedBlockchains := make([]rpc.BlockchainInfo, 0, len(r.Config.blockchains)) - for _, blockchain := range r.Config.blockchains { + ids := make([]int, 0, len(r.Config.blockchains)) + for id := range r.Config.blockchains { ids = append(ids, int(id)) } + sort.Ints(ids) + supportedBlockchains := make([]rpc.BlockchainInfo, 0, len(ids)) + for _, id := range ids { + blockchain := r.Config.blockchains[uint32(id)] supportedBlockchains = append(supportedBlockchains, rpc.BlockchainInfo{ ID: blockchain.ID, Name: blockchain.Name, CustodyAddress: blockchain.ContractAddresses.Custody, AdjudicatorAddress: blockchain.ContractAddresses.Adjudicator, }) }clearnode/blockchain_config_test.go (1)
10-16: Add tests for zero and duplicate chain IDs (enabled blockchains)Guard against config pitfalls and the proposed validation.
@@ func TestBlockchainConfig_verifyVariables(t *testing.T) { { name: "zero chain id", cfg: BlockchainsConfig{ Blockchains: []BlockchainConfig{{ID: 0, Name: "ethereum", Enabled: true}}, }, expectedErrorStr: "invalid chain id 0 for blockchain 'ethereum'", }, { name: "duplicate chain id", cfg: BlockchainsConfig{ Blockchains: []BlockchainConfig{ {ID: 1, Name: "ethereum", Enabled: true}, {ID: 1, Name: "ethereum_copy", Enabled: true}, }, }, expectedErrorStr: "duplicate chain id '1' across enabled blockchains", },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
clearnode/README.md(4 hunks)clearnode/blockchain_config.go(1 hunks)clearnode/blockchain_config_test.go(1 hunks)clearnode/custody.go(3 hunks)clearnode/main.go(2 hunks)clearnode/reconcile_cli.go(4 hunks)clearnode/rpc_router_public.go(1 hunks)clearnode/rpc_router_public_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- clearnode/main.go
- clearnode/README.md
- clearnode/reconcile_cli.go
- clearnode/rpc_router_public_test.go
🧰 Additional context used
🧬 Code graph analysis (4)
clearnode/blockchain_config_test.go (1)
clearnode/blockchain_config.go (3)
BlockchainsConfig(30-33)BlockchainConfig(37-51)ContractAddressesConfig(55-59)
clearnode/rpc_router_public.go (2)
clearnode/pkg/rpc/api.go (2)
BlockchainInfo(494-503)BrokerConfig(486-491)clearnode/config.go (1)
Config(18-23)
clearnode/custody.go (5)
clearnode/nitrolite/bindings.go (2)
NewCustody(147-153)Custody(88-92)clearnode/signer.go (1)
Signer(24-26)clearnode/blockchain_config.go (1)
BlockchainConfig(37-51)clearnode/log.go (1)
Logger(12-35)sdk/src/abis/types.ts (1)
ContractAddresses(6-15)
clearnode/blockchain_config.go (2)
sdk/src/abis/types.ts (1)
ContractAddresses(6-15)clearnode/custody.go (1)
Custody(34-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
clearnode/custody.go (1)
84-90: Nice context-rich logger fieldsIncluding chainID and custodyAddress at construction is helpful.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clearnode/main.go (1)
76-84: Remove unreachable code after Fatal.Line 79 calls
logger.Fatal()which terminates the program, making thecontinuestatement on line 80 unreachable. Remove the continue statement.Apply this diff:
for chainID, blockchain := range config.blockchains { client, err := NewCustody(signer, db, wsNotifier, blockchain, &config.assets, logger) if err != nil { logger.Fatal("failed to initialize blockchain client", "chainID", chainID, "error", err) - continue } custodyClients[chainID] = client go client.ListenEvents(context.Background()) }
♻️ Duplicate comments (1)
clearnode/custody.go (1)
50-96: Constructor validates assetsCfg; also validate contract addresses and RPC.Good nil-check for assetsCfg. Consider validating non-empty RPC and non-zero, valid hex addresses for Custody/Adjudicator/BalanceChecker to fail fast at startup.
Prior review already suggested checking adjudicator zero address.
🧹 Nitpick comments (9)
clearnode/config/migrations/postgres/20251023143424_drop_assets_table.sql (1)
3-3: AddIF EXISTSclause for idempotent migrations.The
DROP TABLEstatement lacks theIF EXISTSclause, which could cause the migration to fail if rerun outside of Goose's control or if the table doesn't exist in certain environments. This is a best-practice safety measure.Apply this diff:
- DROP TABLE assets; + DROP TABLE IF EXISTS assets;clearnode/chart/config/prod/assets.yaml (1)
46-56: Consider adding explicit name for clarity.While the asset configuration validation will default the name to the symbol value ("beatwav"), explicitly setting the name field would improve readability and make the intent clearer.
Apply this diff to add an explicit name:
- symbol: "beatwav" + name: "BeatWav" enabled: trueclearnode/asset_config.go (2)
75-85: Avoid O_SYNC on reads and enable strict YAML field checking.
- O_SYNC has no benefit for read-only opens and can hurt portability/perf. Prefer os.Open.
- Enforce KnownFields to catch typos in assets.yaml at load time.
Apply this diff:
- f, err := os.OpenFile(assetsPath, os.O_RDONLY|os.O_SYNC, 0) + f, err := os.Open(assetsPath) @@ - var cfg AssetsConfig - if err := yaml.NewDecoder(f).Decode(&cfg); err != nil { + var cfg AssetsConfig + dec := yaml.NewDecoder(f) + dec.KnownFields(true) + if err := dec.Decode(&cfg); err != nil { return AssetsConfig{}, err }
115-132: BlockchainID validation is recommended; use common.IsHexAddress cautiously due to behavior change.The review comment's suggestions are partially valid but incomplete:
BlockchainID check: Valid. Adding
token.BlockchainID == 0validation is a sound hardening measure.common.IsHexAddress replacement: Use with caution. The current regex
^0x[0-9a-fA-F]{40}$requires the "0x" prefix, butcommon.IsHexAddressmakes it optional (it strips an optional "0x" and validates the remaining 40 hex chars). This is a subtle behavior change. Decide if accepting addresses without "0x" is intended.Misleading verification instruction: The comment suggests verifying that no other files rely on
contractAddressRegex. However, it is actively used 6 times inclearnode/blockchain_config.go(lines 100, 103, 106, 125, 135, 145). The regex cannot be removed globally; any change is local toasset_config.goonly.If the intent is to maintain strict "0x" prefix validation, stick with the regex. If accepting optional "0x" is acceptable,
common.IsHexAddressis a reasonable choice. Either way, add the BlockchainID check independently.clearnode/custody_test.go (1)
412-420: Repeated decimals scaling boilerplate in tests.Consider a small helper to convert raw -> decimal using asset.Token.Decimals to reduce duplication and mistakes.
Example:
func toUnit(raw decimal.Decimal, d uint8) decimal.Decimal { return raw.Div(decimal.NewFromInt(10).Pow(decimal.NewFromInt(int64(d)))) }Also applies to: 517-527, 602-612, 755-763, 858-866
clearnode/rpc_router_public_test.go (1)
117-123: Order-dependent assertions can be flaky.get_assets may not guarantee ordering; sort results or compare as sets to avoid brittle tests.
Possible tweak:
sort.Slice(responseAssets.Assets, func(i, j int) bool { return responseAssets.Assets[i].Token < responseAssets.Assets[j].Token }) sort.Strings(tc.expectedTokenNames)Also applies to: 136-151
clearnode/custody.go (2)
288-314: Redundant channel ledger +/- before wallet credit.In handleCreated you add then immediately subtract the same amount on channel account before crediting wallet. This nets zero and can be reduced to only the wallet credit plus a single Deposit transaction record.
Apply:
- if err := ledger.Record(channelAccountID, asset.Symbol, amount); err != nil { ... } - if err := ledger.Record(channelAccountID, asset.Symbol, amount.Neg()); err != nil { ... } - ledger = GetWalletLedger(tx, walletAddress) + // Directly credit unified wallet account; channel starts with zero post-create. if err := ledger.Record(walletAccountID, asset.Symbol, amount); err != nil { ... }Behavior stays the same while reducing noise.
616-647: Native balance fetched but not surfaced in metrics.You append rawNativeBalance but never emit a metric for it. Either:
- Add a separate metric for native asset, or
- Drop the fetch to avoid confusion.
Also applies to: 649-675
clearnode/channel_service.go (1)
59-76: Adjudicator address validation (optional).Before using networkConfig.ContractAddresses.Adjudicator, optionally validate it’s a non-zero, valid address to surface config errors earlier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
clearnode/asset.go(0 hunks)clearnode/asset_config.go(1 hunks)clearnode/asset_config_test.go(1 hunks)clearnode/asset_test.go(0 hunks)clearnode/channel_service.go(8 hunks)clearnode/channel_service_test.go(21 hunks)clearnode/chart/config/prod/assets.yaml(1 hunks)clearnode/chart/config/sandbox/assets.yaml(1 hunks)clearnode/chart/config/uat/assets.yaml(1 hunks)clearnode/chart/templates/configmap.yaml(1 hunks)clearnode/chart/templates/deployment.yaml(2 hunks)clearnode/chart/templates/helpers/_common.tpl(1 hunks)clearnode/chart/values.yaml(1 hunks)clearnode/config.go(4 hunks)clearnode/config/compose/example/assets.yaml(1 hunks)clearnode/config/migrations/postgres/20251023143424_drop_assets_table.sql(1 hunks)clearnode/custody.go(11 hunks)clearnode/custody_test.go(11 hunks)clearnode/database.go(1 hunks)clearnode/main.go(2 hunks)clearnode/metrics.go(1 hunks)clearnode/reconcile_cli.go(4 hunks)clearnode/rpc_router.go(1 hunks)clearnode/rpc_router_private_test.go(31 hunks)clearnode/rpc_router_public.go(2 hunks)clearnode/rpc_router_public_test.go(2 hunks)clearnode/rpc_router_test.go(3 hunks)
💤 Files with no reviewable changes (2)
- clearnode/asset.go
- clearnode/asset_test.go
✅ Files skipped from review due to trivial changes (1)
- clearnode/chart/config/sandbox/assets.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- clearnode/chart/values.yaml
🧰 Additional context used
🧬 Code graph analysis (14)
clearnode/reconcile_cli.go (2)
sdk/src/abis/types.ts (1)
ContractAddresses(6-15)clearnode/custody.go (1)
Custody(34-48)
clearnode/database.go (9)
clearnode/ledger.go (2)
Entry(19-28)Entry(30-32)clearnode/channel.go (2)
Channel(22-40)Channel(43-45)clearnode/nitrolite/bindings.go (1)
Channel(40-45)clearnode/app_session.go (2)
AppSession(12-26)AppSession(28-30)clearnode/rpc_store.go (2)
RPCRecord(12-22)RPCRecord(25-27)clearnode/wallet.go (2)
SignerWallet(10-13)SignerWallet(15-17)clearnode/contract_event.go (2)
ContractEvent(16-26)ContractEvent(28-30)clearnode/tag.go (2)
UserTagModel(13-16)UserTagModel(18-20)clearnode/blockchain_action.go (2)
BlockchainAction(26-38)BlockchainAction(40-42)
clearnode/rpc_router_private_test.go (4)
clearnode/config.go (1)
Config(18-24)clearnode/pkg/rpc/api.go (1)
Channel(612-639)clearnode/channel.go (2)
Channel(22-40)Channel(43-45)clearnode/rpc_router_private.go (1)
CreateChannelParams(77-82)
clearnode/asset_config_test.go (1)
clearnode/asset_config.go (4)
AssetsConfig(27-29)AssetConfig(33-44)TokenConfig(48-64)AssetTokenConfig(17-22)
clearnode/channel_service.go (3)
clearnode/blockchain_config.go (1)
BlockchainConfig(37-51)clearnode/asset_config.go (1)
AssetsConfig(27-29)sdk/src/abis/types.ts (1)
ContractAddresses(6-15)
clearnode/rpc_router.go (2)
clearnode/rpc_router_public.go (1)
AssetResponse(15-20)clearnode/config.go (1)
Config(18-24)
clearnode/config.go (2)
clearnode/blockchain_config.go (2)
BlockchainConfig(37-51)LoadBlockchains(70-93)clearnode/asset_config.go (2)
AssetsConfig(27-29)LoadAssets(74-92)
clearnode/channel_service_test.go (6)
clearnode/asset_config.go (4)
AssetsConfig(27-29)AssetTokenConfig(17-22)TokenConfig(48-64)AssetConfig(33-44)clearnode/blockchain_config.go (2)
BlockchainConfig(37-51)ContractAddressesConfig(55-59)clearnode/pkg/rpc/api.go (1)
Channel(612-639)clearnode/channel.go (2)
Channel(22-40)Channel(43-45)clearnode/channel_service.go (1)
NewChannelService(24-26)clearnode/ledger.go (1)
GetWalletLedger(55-57)
clearnode/main.go (3)
clearnode/channel_service.go (1)
NewChannelService(24-26)clearnode/custody.go (1)
NewCustody(51-97)clearnode/nitrolite/bindings.go (1)
NewCustody(147-153)
clearnode/rpc_router_public.go (4)
clearnode/pkg/rpc/api.go (2)
BlockchainInfo(494-503)BrokerConfig(486-491)clearnode/config.go (1)
Config(18-24)sdk/src/abis/types.ts (1)
ContractAddresses(6-15)clearnode/custody.go (1)
Custody(34-48)
clearnode/custody_test.go (2)
clearnode/asset_config.go (3)
AssetsConfig(27-29)AssetConfig(33-44)TokenConfig(48-64)clearnode/ledger.go (1)
GetWalletLedger(55-57)
clearnode/rpc_router_public_test.go (4)
clearnode/blockchain_config.go (2)
BlockchainConfig(37-51)ContractAddressesConfig(55-59)clearnode/pkg/rpc/api.go (1)
BrokerConfig(486-491)clearnode/asset_config.go (1)
TokenConfig(48-64)clearnode/config.go (1)
Config(18-24)
clearnode/rpc_router_test.go (6)
clearnode/blockchain_config.go (2)
BlockchainConfig(37-51)ContractAddressesConfig(55-59)clearnode/custody.go (1)
Custody(34-48)clearnode/config.go (1)
Config(18-24)clearnode/asset_config.go (1)
AssetsConfig(27-29)clearnode/channel_service.go (1)
NewChannelService(24-26)clearnode/rpc_router.go (1)
RPCRouter(16-29)
clearnode/custody.go (3)
clearnode/asset_config.go (1)
AssetsConfig(27-29)clearnode/blockchain_config.go (1)
BlockchainConfig(37-51)clearnode/ledger_transaction.go (1)
RecordLedgerTransaction(39-51)
🪛 YAMLlint (1.37.1)
clearnode/chart/templates/configmap.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (30)
clearnode/config/migrations/postgres/20251023143424_drop_assets_table.sql (1)
1-16: Migration is safe to apply—all verification concerns addressed.Codebase verification confirms:
- No foreign key constraints: Search of all SQL migrations found zero FK constraints referencing the
assetstable.- Data migration completed: All application code references use the config-driven model (
config.assets.Assets), with no direct SQL queries on the assets table found.- Rollback schema correct: The Down migration recreates the exact same schema (token, chain_id, symbol, decimals columns with composite PK) as the initial migration, ensuring proper rollback capability.
clearnode/database.go (1)
258-258: Verify migration path for Asset table removal.The
Assetmodel has been removed from the SQLiteAutoMigratecall, aligning with the shift to YAML-based asset configuration. Ensure that:
- Existing deployments with Asset data have a migration path or data export strategy.
- The Asset table removal is coordinated with the rollout of the new configuration system.
clearnode/chart/templates/deployment.yaml (1)
22-22: LGTM! ConfigMap volume mount properly configured.The changes correctly:
- Add a checksum annotation to trigger pod restarts when config changes
- Mount the config volume at
/app/config(matchingCLEARNODE_CONFIG_DIR_PATH)- Reference the ConfigMap created in
configmap.yamlAlso applies to: 47-49, 53-56
clearnode/config/compose/example/assets.yaml (1)
1-25: LGTM! Example assets configuration looks good.The structure is valid for the new YAML-based asset configuration. Note that all tokens share the same address across different blockchain IDs, which is acceptable for example/test data.
clearnode/rpc_router.go (1)
105-115: LGTM! Clean transition from database-driven to config-driven assets.The changes correctly:
- Replace database query with iteration over in-memory
AssetsConfig- Build
AssetResponseentries from nested asset tokens- Maintain the same response structure for backward compatibility
clearnode/chart/templates/configmap.yaml (1)
1-15: LGTM! ConfigMap template properly structured.The ConfigMap template correctly:
- Uses Helm hooks for pre-install/pre-upgrade execution
- Populates
blockchains.yamlandassets.yamlfrom values- Uses appropriate hook weight and delete policy
Note: The YAMLlint syntax error on line 4 is a false positive—the tool doesn't recognize Helm template syntax (
{{ include ... }}).clearnode/rpc_router_private_test.go (1)
922-923: LGTM! Test refactoring consistently uses seedAsset helper.The tests have been systematically updated to use the
seedAssethelper function, which populatesrouter.Config.assetsinstead of creating database records. This aligns with the config-driven asset approach and maintains consistent test patterns across all scenarios.Also applies to: 991-993, 1068-1069, 1109-1110, 1158-1159, 1198-1199, 1241-1242, 1289-1290, 1338-1339, 1380-1381, 1416-1417, 1464-1465, 1536-1537, 1615-1616, 1666-1667, 1728-1733, 1777-1778, 1820-1821, 1859-1860, 1886-1887, 1914-1915
clearnode/chart/config/uat/assets.yaml (1)
1-25: LGTM! UAT assets configuration properly structured.The assets configuration correctly defines:
- Standard tokens (USDC, wETH) with appropriate decimals (6 and 18)
- Test token (ytest.USD) for Sepolia testnet
- Properly formatted Ethereum addresses
- Correct blockchain IDs (137 for Polygon, 11155111 for Sepolia)
clearnode/metrics.go (1)
174-176: LGTM!The refactoring correctly moves asset management into the Custody struct, eliminating the need to aggregate and pass asset lists at the metrics level. This simplifies the balance metrics update flow and aligns well with the configuration-driven approach.
clearnode/asset_config_test.go (3)
11-101: LGTM!The test function comprehensively validates the asset configuration verification logic, covering error cases (missing symbol, missing/invalid addresses) and inheritance behavior. The note on line 99 appropriately documents the validation side-effect behavior.
103-188: LGTM!The test thoroughly validates the asset token lookup logic, including proper filtering of disabled assets and tokens, as well as verification of all returned fields in the success case.
190-276: LGTM!The test correctly validates the retrieval and filtering logic for getting all tokens on a specific chain, ensuring disabled assets and tokens are properly excluded.
clearnode/chart/config/prod/assets.yaml (1)
57-64: Verify the use of 0xEeeee... address for XRP.The address
0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeEis commonly used as a sentinel value for native blockchain tokens (like ETH on Ethereum). Confirm this is the correct/intended address for XRP on blockchain 1440000, as XRP is typically associated with the XRP Ledger, not an EVM-compatible chain.clearnode/reconcile_cli.go (3)
18-43: LGTM!The refactoring correctly updates the CLI to use blockchain-centric terminology and configuration. The variable naming (chainID) and error messages now properly reflect that this is a blockchain identifier.
60-70: LGTM!The custody initialization correctly uses the new signature with BlockchainConfig and AssetsConfig, simplifying the parameter passing by consolidating network-specific fields into the blockchain object.
72-86: LGTM!The ReconcileBlockRange call correctly accesses contract addresses and configuration through the blockchain object's structured fields.
clearnode/rpc_router_test.go (2)
114-136: LGTM!The test setup correctly reflects the new BlockchainConfig structure with nested ContractAddressesConfig. The initialization of ChannelService and Config properly uses the blockchains map and assets configuration.
29-29: LGTM!The AutoMigrate call correctly includes the BlockchainAction model alongside other models.
clearnode/config.go (3)
30-40: LGTM!The configurable directory path for loading configuration files improves flexibility and allows for environment-specific configurations. The default value of "." maintains backward compatibility.
49-54: LGTM!The improved error handling now properly returns the parsing error instead of just logging it, allowing the caller to handle the failure appropriately.
79-87: LGTM!The centralized loading of blockchains and assets from YAML files aligns with the PR's goal of streamlining configuration. Using Fatal for these critical configuration failures is appropriate as confirmed in the past review discussion.
clearnode/rpc_router_public.go (2)
113-127: LGTM!The refactoring correctly builds blockchain information from the config-driven blockchains map, eliminating database dependency for this static configuration data. The use of canonical rpc.BlockchainInfo and rpc.BrokerConfig types prevents duplication.
144-158: LGTM!The asset retrieval now correctly uses the config-driven AssetsConfig, iterating over assets and their tokens with optional chain ID filtering. This eliminates the database query overhead for static configuration data.
clearnode/main.go (1)
62-62: LGTM!The ChannelService initialization correctly uses the new blockchain-centric configuration structure, passing both blockchains map and assets configuration.
clearnode/custody_test.go (1)
71-86: Nice focused seed config for tests.Inline AssetsConfig seeding makes tests independent of YAML. Looks good.
clearnode/rpc_router_public_test.go (1)
53-61: Config wiring assertions look correct.blockchains map with IDs and contract addresses aligns with new structs.
clearnode/channel_service_test.go (4)
111-121: LGTM!The blockchain configuration setup correctly uses the new
BlockchainConfigstructure with proper contract addresses for testing.
366-366: LGTM!The adjudicator address reference correctly uses the new
BlockchainConfig.ContractAddresses.Adjudicatorpath.
94-499: Well-structured test suite with comprehensive coverage.The test functions properly cover success paths, error conditions, and edge cases for channel operations. The migration to the new
BlockchainConfigandAssetsConfigstructures is consistent throughout.
140-140: No actionable issues found. Review comment is incorrect.The review comment assumes
RequestResizeandRequestClosemethods access theblockchainsfield, but code inspection shows they do not. Both methods only accesss.db,s.assetsCfg, ands.signer. Theblockchainsfield is accessed only inRequestCreate(line 59), which is tested with an actual blockchains map (not nil), as noted in the review comment itself. Passingnilblockchains toRequestResizeandRequestClosetests is safe since these methods never reference that field.Likely an incorrect or invalid review comment.
alessio
left a comment
There was a problem hiding this comment.
Pending comments to be incorporated.
Excellent work
9b8505c to
45018f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
clearnode/blockchain_config.go (1)
22-25: Consider adding brief inline comments to explain regex patterns.While the struct field comments document the expected formats, adding brief inline comments here would improve maintainability. For example:
var ( + // blockchainNameRegex matches snake_case identifiers (min 3 chars: letter, underscore/letters, letter) blockchainNameRegex = regexp.MustCompile(`^[a-z][a-z_]+[a-z]$`) + // contractAddressRegex matches Ethereum addresses (0x + 40 hex chars) contractAddressRegex = regexp.MustCompile(`^0x[0-9a-fA-F]{40}$`) )Based on past review comments.
clearnode/channel_service.go (1)
23-26: Consider adding defensive nil check for assetsCfg.While the constructor is called from main.go where config loading is guaranteed, adding a nil check would prevent potential panics during testing or refactoring:
func NewChannelService(db *gorm.DB, blockchains map[uint32]BlockchainConfig, assetsCfg *AssetsConfig, signer *Signer) *ChannelService { + if assetsCfg == nil { + panic("assetsCfg must not be nil") + } return &ChannelService{db: db, blockchains: blockchains, assetsCfg: assetsCfg, signer: signer} }Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
clearnode/asset_config.go(1 hunks)clearnode/asset_config_test.go(1 hunks)clearnode/blockchain_config.go(1 hunks)clearnode/channel_service.go(8 hunks)clearnode/chart/templates/helpers/_common.tpl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- clearnode/chart/templates/helpers/_common.tpl
🧰 Additional context used
🧬 Code graph analysis (3)
clearnode/channel_service.go (3)
clearnode/blockchain_config.go (1)
BlockchainConfig(37-51)clearnode/asset_config.go (1)
AssetsConfig(28-30)sdk/src/abis/types.ts (1)
ContractAddresses(6-15)
clearnode/blockchain_config.go (2)
sdk/src/abis/types.ts (1)
ContractAddresses(6-15)clearnode/custody.go (1)
Custody(34-48)
clearnode/asset_config_test.go (1)
clearnode/asset_config.go (4)
AssetsConfig(28-30)AssetConfig(34-45)TokenConfig(49-65)AssetTokenConfig(18-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test (Integration) / Test Integration
🔇 Additional comments (18)
clearnode/blockchain_config.go (7)
16-20: LGTM!Constants are well-defined and appropriately scoped.
27-59: LGTM!Type definitions are well-structured with comprehensive documentation.
61-93: LGTM!LoadBlockchains implements proper file handling, validation, and returns a clean interface.
95-155: LGTM!Comprehensive validation with proper default cascading and clear error messages.
157-180: LGTM!RPC verification ensures correct network connectivity and chain ID matching.
182-192: LGTM!Clean filtering of enabled blockchains into result map.
194-217: LGTM!Proper RPC connection handling with timeout and resource cleanup.
clearnode/asset_config_test.go (3)
10-101: LGTM!Comprehensive test coverage for validation and default inheritance logic.
103-188: LGTM!Thorough test coverage for asset token lookup including all edge cases and filtering logic.
190-276: LGTM!Comprehensive test for chain-specific token retrieval with proper filtering validation.
clearnode/channel_service.go (3)
28-112: LGTM!Proper integration with new blockchains and assetsCfg configuration.
114-221: LGTM!Correctly uses asset configuration for token metadata and balance calculations.
223-310: LGTM!Proper asset lookup and token decimals usage in close operation.
clearnode/asset_config.go (5)
12-65: LGTM!Well-structured type definitions with comprehensive documentation explaining inheritance behavior.
67-93: LGTM!Clean configuration loading with proper validation and error handling.
95-139: LGTM!Validation correctly applies defaults through indexed slice access and inheritance is properly persisted.
141-168: LGTM!Case-insensitive address lookup correctly handles checksummed addresses and filters disabled entries.
170-192: LGTM!Clean chain-specific token retrieval with proper filtering.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
clearnode/channel_service_test.go (1)
15-48: Consider adding duplicate token check in test helper.The
seedAssetfunction unconditionally appends tokens (line 45) without checking if an identical token (same address and chain ID) already exists. While not critical in the current test suite, adding a duplicate check would prevent unexpected behavior if tests are refactored:if index == -1 { assetsCfg.Assets = append(assetsCfg.Assets, AssetConfig{ Name: symbol, Symbol: symbol, }) index = len(assetsCfg.Assets) - 1 } + + // Check for existing token to prevent duplicates + for _, existingToken := range assetsCfg.Assets[index].Tokens { + if existingToken.BlockchainID == chainID && strings.EqualFold(existingToken.Address, tokenAddress) { + return AssetTokenConfig{ + Name: symbol, + Symbol: symbol, + Token: existingToken, + } + } + } + assetsCfg.Assets[index].Tokens = append(assetsCfg.Assets[index].Tokens, token)
🧹 Nitpick comments (1)
clearnode/blockchain_config.go (1)
23-24: Consider adding brief comments to regex patterns.While the patterns are straightforward, adding brief comments explaining the expected format would improve maintainability:
var ( + // blockchainNameRegex validates snake_case blockchain names (lowercase letters and underscores) blockchainNameRegex = regexp.MustCompile(`^[a-z][a-z_]+[a-z]$`) + // contractAddressRegex validates Ethereum addresses (0x followed by 40 hex characters) contractAddressRegex = regexp.MustCompile(`^0x[0-9a-fA-F]{40}$`) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
clearnode/asset_config.go(1 hunks)clearnode/asset_config_test.go(1 hunks)clearnode/blockchain_config.go(1 hunks)clearnode/blockchain_config_test.go(1 hunks)clearnode/channel_service_test.go(21 hunks)clearnode/custody_test.go(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- clearnode/asset_config_test.go
- clearnode/blockchain_config_test.go
🧰 Additional context used
🧬 Code graph analysis (3)
clearnode/blockchain_config.go (2)
sdk/src/abis/types.ts (1)
ContractAddresses(6-15)clearnode/custody.go (1)
Custody(34-48)
clearnode/custody_test.go (2)
clearnode/asset_config.go (3)
AssetsConfig(32-34)AssetConfig(38-49)TokenConfig(53-69)clearnode/ledger.go (1)
GetWalletLedger(55-57)
clearnode/channel_service_test.go (4)
clearnode/asset_config.go (4)
AssetsConfig(32-34)AssetTokenConfig(18-27)TokenConfig(53-69)AssetConfig(38-49)clearnode/blockchain_config.go (2)
BlockchainConfig(37-51)ContractAddressesConfig(55-59)clearnode/channel.go (2)
Channel(22-40)Channel(43-45)clearnode/channel_service.go (1)
NewChannelService(24-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
clearnode/asset_config.go (2)
107-143: LGTM! Slice mutation issue properly fixed.The indexed assignments to
cfg.Assets[i].Name,cfg.Assets[i].Tokens[j].Symbol, andcfg.Assets[i].Tokens[j].Namecorrectly write defaults through to the underlying slices, ensuring inheritance is persisted for downstream code.
160-160: LGTM! Case-insensitive address comparison implemented.Using
strings.EqualFoldensures EIP-55 checksummed addresses match correctly regardless of case variation.clearnode/custody_test.go (2)
71-98: LGTM! Clean migration to assetsCfg-based configuration.The test setup correctly constructs
AssetsConfigwith token decimals and passes it to theCustodystruct, aligning with the PR's goal of streamlining asset configuration.
410-418: LGTM! Consistent asset lookup and decimal handling.The pattern of using
assetsCfg.GetAssetTokenByAddressAndChainIDfollowed byasset.Token.Decimalsfor decimal conversions is applied uniformly across all test cases.clearnode/blockchain_config.go (2)
201-205: LGTM! Resource leak fixed with deferred close.The ethclient is now properly closed via
defer client.Close(), preventing socket leaks during RPC verification.
173-173: LGTM! Error message improved for clarity.The error now explicitly mentions "ChainID check failed", making it easier to identify and troubleshoot RPC connection issues.
clearnode/channel_service_test.go (1)
108-118: LGTM! Clean migration to BlockchainConfig-based setup.The test setup correctly constructs
blockchainsmap withContractAddressesConfigand propagates it through service initialization, aligning with the PR's YAML-driven configuration approach.
db470c1 to
7b85d20
Compare
| | `DATABASE_DRIVER` | Database driver to use (postgres/sqlite) | No | sqlite | | ||
| | `CLEARNODE_DATABASE_URL` | Database connection string | No | clearnode.db | | ||
| | `LOG_LEVEL` | Logging level (debug, info, warn, error) | No | info | | ||
| | `CLEARNODE_LOG_LEVEL` | Logging level (debug, info, warn, error) | No | info | |
There was a problem hiding this comment.
I think it'd be great for the application to infer configuration from environment variables whose names are prefixed with CLEARNODE_. It's universally recognized as a good practice as it 1. makes envvars lookups easier 2. prevent environment pollution
There was a problem hiding this comment.
Bear in mind: I am not advocating for such envvar parsing refactor to take place in the same context of this work. I'd suggest though to either open an issue or create a user story for it. Having a public Github issue comes with the benefit that some third-party open source contributor could pick it up and get it done
There was a problem hiding this comment.
Do I understand you correctly, you want all our environment variables' names to have CLEARNODE_ prefix?
| name: Deploy to Prod | ||
| uses: ./.github/workflows/deploy-clearnode.yaml | ||
| with: | ||
| target-env: prod | ||
| secrets: | ||
| gke-project: ${{ secrets.GKE_PROJECT }} | ||
| gke-sa-key: ${{ secrets.GKE_PROD_SA_KEY }} | ||
|
|
||
| deploy-sandbox: |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
clearnode/channel_service_test.go (1)
15-48: Consider adding duplicate check for test robustness.The function unconditionally appends tokens (line 45) without checking if an identical token (same address and chain ID) already exists for the asset. While current tests avoid this by creating fresh
assetsCfginstances and seeding unique tokens, the helper is fragile—future tests could inadvertently create duplicate tokens, leading to unexpected behavior.If you want to make the helper more robust, add a duplicate check before appending:
if index == -1 { assetsCfg.Assets = append(assetsCfg.Assets, AssetConfig{ Name: symbol, Symbol: symbol, }) index = len(assetsCfg.Assets) - 1 } + + // Check if token already exists for this blockchain + for _, existingToken := range assetsCfg.Assets[index].Tokens { + if existingToken.BlockchainID == chainID && strings.EqualFold(existingToken.Address, tokenAddress) { + return AssetTokenConfig{ + Name: symbol, + Symbol: symbol, + Token: existingToken, + } + } + } + assetsCfg.Assets[index].Tokens = append(assetsCfg.Assets[index].Tokens, token) return asset
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/deploy-clearnode.yaml(2 hunks).github/workflows/main-pr.yml(1 hunks).github/workflows/main-push.yml(2 hunks)clearnode/asset_config.go(1 hunks)clearnode/asset_config_test.go(1 hunks)clearnode/blockchain_config.go(1 hunks)clearnode/blockchain_config_test.go(1 hunks)clearnode/channel_service_test.go(21 hunks)clearnode/chart/config/prod/assets.yaml(1 hunks)clearnode/chart/config/prod/blockchains.yaml(1 hunks)clearnode/chart/config/sandbox/assets.yaml(1 hunks)clearnode/chart/config/sandbox/blockchains.yaml(1 hunks)clearnode/chart/config/uat/assets.yaml(1 hunks)clearnode/chart/config/uat/blockchains.yaml(1 hunks)clearnode/chart/values.yaml(1 hunks)clearnode/config/compose/example/assets.yaml(1 hunks)clearnode/config/compose/example/blockchains.yaml(1 hunks)clearnode/custody_test.go(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- clearnode/chart/config/prod/blockchains.yaml
- clearnode/chart/config/prod/assets.yaml
- clearnode/chart/config/uat/blockchains.yaml
- clearnode/config/compose/example/assets.yaml
- clearnode/config/compose/example/blockchains.yaml
- clearnode/chart/config/sandbox/assets.yaml
- clearnode/chart/config/uat/assets.yaml
🧰 Additional context used
🧬 Code graph analysis (5)
clearnode/asset_config_test.go (1)
clearnode/asset_config.go (4)
AssetsConfig(32-34)AssetConfig(38-49)TokenConfig(53-69)AssetTokenConfig(18-27)
clearnode/blockchain_config.go (1)
clearnode/custody.go (1)
Custody(34-48)
clearnode/blockchain_config_test.go (1)
clearnode/blockchain_config.go (3)
BlockchainsConfig(30-33)BlockchainConfig(37-51)ContractAddressesConfig(55-59)
clearnode/channel_service_test.go (5)
clearnode/asset_config.go (4)
AssetsConfig(32-34)AssetTokenConfig(18-27)TokenConfig(53-69)AssetConfig(38-49)clearnode/blockchain_config.go (2)
BlockchainConfig(37-51)ContractAddressesConfig(55-59)clearnode/channel.go (2)
Channel(22-40)Channel(43-45)clearnode/channel_service.go (1)
NewChannelService(24-26)clearnode/ledger.go (1)
GetWalletLedger(55-57)
clearnode/custody_test.go (2)
clearnode/asset_config.go (3)
AssetsConfig(32-34)AssetConfig(38-49)TokenConfig(53-69)clearnode/ledger.go (1)
GetWalletLedger(55-57)
🪛 GitHub Check: CodeQL
.github/workflows/main-push.yml
[warning] 109-117: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{}}
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
clearnode/chart/values.yaml (1)
34-37: LGTM!The addition of
blockchainsandassetsconfiguration keys is clean and well-documented. These fields provide the foundation for the new YAML-driven configuration approach.clearnode/chart/config/sandbox/blockchains.yaml (1)
1-28: Configuration looks valid overall.The blockchain definitions and contract addresses follow the expected structure.
However, note that
xrpl_evm_testnethasblock_step: 499, which is significantly smaller than the default of 10,000 (as seen in other blockchains that don't specify this field). Please verify this is intentional and appropriate for the XRPL EVM testnet's block production rate or other constraints..github/workflows/main-pr.yml (1)
8-15: LGTM!The deployment job is properly configured with the necessary target environment and secrets.
clearnode/asset_config_test.go (1)
1-269: Excellent test coverage!The test suite comprehensively validates the new asset configuration system:
- Validation logic (missing/invalid data, inheritance)
- Lookup by address and chain ID (disabled filtering, data structure)
- Retrieval by chain ID (disabled filtering)
The test cases are well-structured and cover edge cases appropriately.
clearnode/blockchain_config_test.go (1)
1-266: Comprehensive validation test coverage!The test suite thoroughly validates the blockchain configuration system:
- Valid configurations with default fallbacks
- Name validation (snake_case enforcement)
- Contract address validation (custody, adjudicator, balance_checker)
- Proper handling of disabled blockchains
The test cases are well-organized and cover both success and failure scenarios appropriately.
clearnode/custody_test.go (2)
71-85: Clean migration to config-based assets!The test setup now uses
AssetsConfiginstead of inline asset data, aligning with the new YAML-driven configuration approach. The structure is clear and maintainable.Also applies to: 98-98
410-411: Consistent asset lookup and decimal handling throughout.All test cases correctly:
- Use
GetAssetTokenByAddressAndChainIDfor asset lookup- Access decimals via
asset.Token.Decimals- Handle the nested
AssetTokenConfigstructure appropriatelyThe migration is thorough and consistent.
Also applies to: 414-417, 516-517, 520-524, 600-601, 604-609, 753-757, 856-860
clearnode/asset_config.go (2)
107-143: LGTM! Range-by-value issue correctly addressed.The default inheritance logic now correctly persists changes by directly indexing into slices (
cfg.Assets[i].Name,cfg.Assets[i].Tokens[j].Symbol) rather than assigning to loop variables. The re-fetch pattern on lines 120 and 133 ensures validation uses the updated values.
145-171: LGTM! Case-insensitive address comparison implemented.The method correctly uses
strings.EqualFoldon line 160 to handle EIP-55 checksummed addresses, ensuring lookups succeed regardless of address casing.clearnode/blockchain_config.go (2)
194-217: LGTM! Resource leak correctly addressed.The
defer client.Close()on line 205 ensures the ethclient connection is properly cleaned up, preventing socket leaks.
98-155: LGTM! Configuration validation and defaults correctly applied.The method properly validates contract address formats, applies default addresses when blockchain-specific values are missing, and sets the default block step. The in-place mutations via direct slice indexing ensure defaults are persisted correctly.
clearnode/channel_service_test.go (1)
91-496: LGTM! Tests systematically updated for new config structures.The tests correctly use the new
BlockchainConfig,AssetsConfig, andAssetTokenConfigtypes. Token addresses are properly accessed viaasset.Token.Address, blockchain contract addresses viablockchains[chainID].ContractAddresses, and theNewChannelServiceconstructor signature is updated consistently across all tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
clearnode/README.md (1)
11-11: Enhancement: Consider adding benefits of YAML-based configuration (optional).The feature description now mentions YAML-based configuration, which is good. To further emphasize the value of this approach, consider briefly mentioning benefits such as improved readability, maintainability, and easier version control compared to environment-variable-based setup.
Example enhancement:
-- **Multi-Chain Support**: Connect to multiple EVM blockchains with YAML-based configuration +- **Multi-Chain Support**: Connect to multiple EVM blockchains with YAML-based configuration for improved readability and maintainability
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/deploy-clearnode.yaml(2 hunks)clearnode/README.md(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/deploy-clearnode.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
clearnode/README.md (3)
132-179: Well-structured documentation for new configuration system.The Blockchain Configuration, RPC Configuration, and updated Environment Variables sections clearly document the shift to YAML-driven configuration. Past feedback has been well-integrated:
CONFIG_DIR_PATHgeneralized to "configuration files" (addressing prior concern about only mentioning blockchains.yaml)- Environment variable naming follows established conventions (e.g.,
CLEARNODE_LOG_LEVEL)- RPC endpoint pattern clearly documented
- Example configuration reference provided
185-211: Quick Start instructions align with new configuration approach.The updated Quick Start section properly guides users through:
- Setting up a local configuration directory
- Configuring blockchains.yaml and assets.yaml
- Running with Docker Compose
Instructions are clear and consistent with the documented configuration system.
136-136: Example configuration file reference is valid.The referenced example file exists at
./clearnode/config/compose/example/blockchains.yaml, confirming the documentation link is accurate and users can access the referenced configuration example.
nksazonov
left a comment
There was a problem hiding this comment.
Great work!
Thanks for addressing comments!
Summary by CodeRabbit
New Features
Refactor
Chores
Documentation
Tests