Conversation
dimast-x
requested review from
alessio,
ihsraham,
nksazonov and
philanton
as code owners
dimast-x
changed the title
dimast-x
force-pushed
the
feat/integrate-locking
branch
from
5c9194d to
70fbdcc
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds on-chain locking support: new LockingClient, LockingContractReactor, events/types for user locked balances, configuration for per-blockchain locking addresses and validators, renames core client/event handler types (Client→BlockchainClient, BlockchainEventHandler→ChannelHubEventHandler), and wires SDKs, CLI, and memory store to support locking flows. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client/API
participant LockC as LockingClient
participant EVM as EVM Network
participant Listener as Listener
participant Reactor as LockingContractReactor
participant Handler as EventHandlerService
participant Store as DB Store
UI->>LockC: Lock(target, amount)
LockC->>EVM: Send Lock tx
EVM->>EVM: Execute tx, emit Locked event
EVM-->>Listener: New Log
Listener->>Reactor: HandleEvent(ctx, log)
Reactor->>Reactor: Parse Locked -> build UserLockedBalanceUpdatedEvent
Reactor->>Handler: HandleUserLockedBalanceUpdated(ctx, event)
Handler->>Store: UpdateUserStaked(wallet, blockchainID, balance)
Store-->>Handler: ok / error
Handler-->>Reactor: return
Reactor->>Listener: OnEventProcessed(success)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~80 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/rpc/types.go (1)
278-288:⚠️ Potential issue | 🔴 CriticalAdd missing field mapping to fix SDK locking contract operations.
The
LockingContractAddressfield is added toBlockchainInfoV1, but the RPC handler's mapping function inclearnode/api/node_v1/utils.godoes not populate it. This causes the SDK'sgetLockingContractAddressto fail with "locking contract address not configured" since the field will always be empty in API responses.Fix in clearnode/api/node_v1/utils.go
func mapBlockchainV1(blockchain core.Blockchain) rpc.BlockchainInfoV1 { return rpc.BlockchainInfoV1{ Name: blockchain.Name, BlockchainID: strconv.FormatUint(blockchain.ID, 10), ChannelHubAddress: blockchain.ChannelHubAddress, + LockingContractAddress: blockchain.LockingContractAddress, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rpc/types.go` around lines 278 - 288, The new BlockchainInfoV1.LockingContractAddress field is never populated by the RPC mapping function in clearnode/api/node_v1/utils.go, causing SDK lookups to fail; update that mapping function to set BlockchainInfoV1.LockingContractAddress from the source blockchain model's locking contract value (e.g., source.LockingContractAddress or equivalent field) when constructing the BlockchainInfoV1 instance so API responses include the configured locking contract address.pkg/blockchain/evm/channel_hub_reactor.go (1)
58-66:⚠️ Potential issue | 🔴 CriticalUse the ChannelHub event map here.
Line 62 still indexes
eventMapping, but this file now populateschannelHubEventMapping. BecauseeventMappingexists inpkg/blockchain/evm/app_registry_reactor.go, this compiles but ChannelHub logs are resolved against the locking-contract map and get dropped as “unknown event”.🐛 Suggested fix
- eventName, ok := eventMapping[eventID] + eventName, ok := channelHubEventMapping[eventID]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/blockchain/evm/channel_hub_reactor.go` around lines 58 - 66, The ChannelHubReactor.HandleEvent is looking up events in the wrong map: it uses eventMapping instead of the ChannelHub-specific channelHubEventMapping, so ChannelHub logs are misclassified; update the lookup in HandleEvent (where eventID := l.Topics[0] and eventName, ok := eventMapping[eventID]) to use channelHubEventMapping (eventName, ok := channelHubEventMapping[eventID]) so ChannelHub events are resolved against the correct map.
🧹 Nitpick comments (4)
clearnode/store/database/database.go (1)
70-78: TODO may be redundant—logger is already configured to suppress SQL.The logger is set to
logger.Warnmode on line 78, which should already prevent GORM from printing SQL statements (they're logged at Info level). If SQL queries are still appearing in logs, this might indicate a configuration issue elsewhere. If the goal is to completely disable logging, considerlogger.Silentmode instead.Would you like me to help verify if SQL is still being logged despite the Warn level, or assist in implementing a stricter logging configuration?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/database/database.go` around lines 70 - 78, The current GORM config sets Logger: logger.Default.LogMode(logger.Warn) which should suppress SQL but if you want to guarantee SQL is not printed change the logger to logger.Default.LogMode(logger.Silent) or construct a custom logger via logger.New(...) with desired LogLevel and io.Writer; locate the gorm.Open call and replace Logger: logger.Default.LogMode(logger.Warn) with Logger: logger.Default.LogMode(logger.Silent) (or inject the custom logger) and keep other options like schema.NamingStrategy and PrepareStmt unchanged.pkg/core/interface.go (1)
13-13: Preserve compatibility for the renamed exported interfaces.
Client→BlockchainClientandBlockchainEventHandler→ChannelHubEventHandleris a breaking API rename for any external package importingpkg/core, even though the method sets are effectively unchanged. If this package is part of the public surface, keep deprecated type aliases until the next major release.♻️ Suggested compatibility shim
type BlockchainClient interface { ... } + +// Deprecated: use BlockchainClient. +type Client = BlockchainClient ... type ChannelHubEventHandler interface { ... } + +// Deprecated: use ChannelHubEventHandler. +type BlockchainEventHandler = ChannelHubEventHandlerAlso applies to: 88-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/core/interface.go` at line 13, Add backward-compatible type aliases for the renamed exported interfaces: declare `type Client = BlockchainClient` and `type BlockchainEventHandler = ChannelHubEventHandler` alongside the new interface declarations so external packages won't break; include brief deprecation comments on these aliases noting they will be removed in the next major release and ensure the aliases are exported (capitalized) and placed in the same package near the original `BlockchainClient` and `ChannelHubEventHandler` declarations.clearnode/store/memory/blockchain_config_test.go (1)
18-51: Please add a failing case for the new locking-address validation.This file now covers
LockingContractAddressonly on the happy path. A malformedlocking_contract_addressand a config with neither contract address would exercise the new validation rules and catch regressions much earlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/memory/blockchain_config_test.go` around lines 18 - 51, Add failing test cases to blockchain_config_test.go to cover the new LockingContractAddress validation: create one subtest where a BlockchainConfig has an invalid/malformed LockingContractAddress (e.g., not a 0x hex address) and assert the parser/validator returns an error matching the validation message, and another subtest where a BlockchainConfig has neither ChannelHubAddress nor LockingContractAddress and assert it fails; update the table-driven tests that construct BlockchainsConfig / Blockchains to include these cases and assert errors are produced by the same code paths that handle BlockchainsConfig validation (reference BlockchainsConfig, BlockchainConfig, LockingContractAddress, ChannelHubAddress, defaultBlockStep).pkg/blockchain/evm/blockchain_client.go (1)
24-24: Make the constructor address name explicit.Now that this package deals with ChannelHub and Locking/AppRegistry contracts,
contractAddressis the one remaining ambiguous name on the public constructor. Renaming it tochannelHubContractAddresswould make the API match the field and reduce accidental mix-ups.♻️ Suggested rename
func NewBlockchainClient( - contractAddress common.Address, + channelHubContractAddress common.Address, evmClient EVMClient, txSigner sign.Signer, blockchainID uint64, nodeAddress string, assetStore AssetStore, opts ...ClientOption, ) (*BlockchainClient, error) { - contract, err := NewChannelHub(contractAddress, evmClient) + contract, err := NewChannelHub(channelHubContractAddress, evmClient) if err != nil { return nil, errors.Wrap(err, "failed to create ChannelHub contract instance") } client := &BlockchainClient{ @@ - channelHubContractAddress: contractAddress, + channelHubContractAddress: channelHubContractAddress, assetStore: assetStore,Also applies to: 36-37, 57-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/blockchain/evm/blockchain_client.go` at line 24, Rename the ambiguous constructor parameter name `contractAddress` to `channelHubContractAddress` across the public constructor and all usages so it matches the struct field `channelHubContractAddress` and avoids confusion with other contracts; update the constructor signature (e.g., NewBlockchainClient), its parameter list, and all references/pass-throughs at the call sites and assignments (lines around where `channelHubContractAddress` is set and where `contractAddress` was used, including the places mentioned at 36-37 and 57) to use `channelHubContractAddress` consistently.
🤖 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/app_registry_reactor_test.go`:
- Around line 111-117: The test's error-path assertion is weak because
processedSuccess is initialized to false so the assertion can pass even if the
SetOnEventProcessed callback never runs; update the test around
SetOnEventProcessed / processedSuccess and HandleEvent to either initialize
processedSuccess to true and assert it becomes false, or introduce a separate
boolean like processedCalled (or a counter) that is set inside the callback and
assert both that the callback was invoked and that processedSuccess is false
after calling reactor.HandleEvent; reference the processedSuccess variable,
SetOnEventProcessed(func(_ uint64, success bool){...}), and
reactor.HandleEvent(...) when making the change.
- Around line 68-72: The test currently compares checksummed addresses via
common.HexToAddress(capturedEvent.UserAddress).String(), which masks whether
UserAddress was lowercased; update the assertion to explicitly verify
lowercasing by comparing capturedEvent.UserAddress to the lowercase hex form of
the expected address (derive from userAddr, e.g. strings.ToLower(userAddr.Hex())
or userAddr.String() lowercased). Modify the assertion that references
capturedEvent.UserAddress (and remove or replace the call to common.HexToAddress
there) so the test fails if the reactor stops storing a lowercase address.
In `@pkg/blockchain/evm/app_registry_reactor.go`:
- Around line 89-107: The code currently calls r.storeContractEvent and logs
"processed event" even when handleLocked/handleWithdrawn returned an error,
which advances the resume cursor on failure; update the control flow so that
r.storeContractEvent(...) and logger.Info("processed event", ...) are executed
only when err == nil (i.e., after successful handling), leaving failed events
unpersisted so the listener can retry; keep onEventProcessed(...) behavior as
needed but ensure storage/logging of successful processing happens only on
success (refer to r.storeContractEvent, handleLocked, handleWithdrawn,
onEventProcessed, and logger.Info).
In `@pkg/blockchain/evm/blockchain_client.go`:
- Line 236: The response currently sets the response field "Node" to
c.channelHubContractAddress.Hex(), which overloads the Node field (used by
GetHomeChannelData as the participant node address) with a contract address;
update the code that builds this escrow response to set Node to
c.nodeAddress.Hex() instead (or alternatively rename the field to something
contract-specific like ChannelHubContract if you intend to keep the contract
address), and apply the same change to the other occurrence mentioned (around
the second instance at the 261 region) so callers see the actual node address
rather than the contract.
In `@pkg/blockchain/evm/init.go`:
- Around line 3-6: The package init currently calls initChannelHub() and
initLockingContract() which eagerly parse ChannelHubABI and lockingContractAbi
on import; change this so each ABI is parsed lazily using a sync.Once guarded
initializer: add a sync.Once (e.g., channelHubOnce and lockingContractOnce) and
move parsing logic into functions that call channelHubOnce.Do(initChannelHub)
and lockingContractOnce.Do(initLockingContract), remove their invocation from
init(), and ensure callers that need the ABI (places referencing ChannelHubABI
or lockingContractAbi, and code that checks b.LockingContractAddress) invoke the
corresponding lazy initializer before use so the locking contract ABI is only
parsed when the contract is actually configured.
In `@pkg/blockchain/evm/locking_client.go`:
- Around line 92-109: The Lock, Withdraw and GetBalance methods call
common.HexToAddress on user-supplied strings without validation; update each
method (Lock, Withdraw, GetBalance) to first call
common.IsHexAddress(targetWalletAddress) (or the corresponding param name) and
return a clear error when false, only then call common.HexToAddress; ensure the
error path returns before any contract calls or conversions so malformed input
is rejected instead of being silently converted.
- Around line 98-99: Change decimalToBigInt to return (*big.Int, error) and
validate input: reject negative amounts and after shifting by tokenDecimals
ensure no fractional remainder (return an error on precision loss). Update
callers in locking client where decimalToBigInt is invoked (the call assigning
amountBig and the later call at the other site) to capture the error, propagate
or return it (or wrap contextually), and avoid proceeding when error != nil;
ensure variable names (amountBig, decimalToBigInt, tokenDecimals, and any
enclosing methods) are used to locate and update the call sites.
In `@sdk/ts/src/client.ts`:
- Around line 1748-1760: The Node path is passing only an address string to
createWalletClient which causes viem to attempt RPC signing; instead extract the
private key from this.txSigner (which is an EthereumRawSigner or exposes the raw
private key) and pass the full account object/privateKey into createWalletClient
in the non-browser branch (the walletClient creation block using transport:
http(rpcUrl)); update the Node createWalletClient call to supply the account
object (containing the private key) rather than this.txSigner.getAddress() so
transactions are signed locally.
---
Outside diff comments:
In `@pkg/blockchain/evm/channel_hub_reactor.go`:
- Around line 58-66: The ChannelHubReactor.HandleEvent is looking up events in
the wrong map: it uses eventMapping instead of the ChannelHub-specific
channelHubEventMapping, so ChannelHub logs are misclassified; update the lookup
in HandleEvent (where eventID := l.Topics[0] and eventName, ok :=
eventMapping[eventID]) to use channelHubEventMapping (eventName, ok :=
channelHubEventMapping[eventID]) so ChannelHub events are resolved against the
correct map.
In `@pkg/rpc/types.go`:
- Around line 278-288: The new BlockchainInfoV1.LockingContractAddress field is
never populated by the RPC mapping function in clearnode/api/node_v1/utils.go,
causing SDK lookups to fail; update that mapping function to set
BlockchainInfoV1.LockingContractAddress from the source blockchain model's
locking contract value (e.g., source.LockingContractAddress or equivalent field)
when constructing the BlockchainInfoV1 instance so API responses include the
configured locking contract address.
---
Nitpick comments:
In `@clearnode/store/database/database.go`:
- Around line 70-78: The current GORM config sets Logger:
logger.Default.LogMode(logger.Warn) which should suppress SQL but if you want to
guarantee SQL is not printed change the logger to
logger.Default.LogMode(logger.Silent) or construct a custom logger via
logger.New(...) with desired LogLevel and io.Writer; locate the gorm.Open call
and replace Logger: logger.Default.LogMode(logger.Warn) with Logger:
logger.Default.LogMode(logger.Silent) (or inject the custom logger) and keep
other options like schema.NamingStrategy and PrepareStmt unchanged.
In `@clearnode/store/memory/blockchain_config_test.go`:
- Around line 18-51: Add failing test cases to blockchain_config_test.go to
cover the new LockingContractAddress validation: create one subtest where a
BlockchainConfig has an invalid/malformed LockingContractAddress (e.g., not a 0x
hex address) and assert the parser/validator returns an error matching the
validation message, and another subtest where a BlockchainConfig has neither
ChannelHubAddress nor LockingContractAddress and assert it fails; update the
table-driven tests that construct BlockchainsConfig / Blockchains to include
these cases and assert errors are produced by the same code paths that handle
BlockchainsConfig validation (reference BlockchainsConfig, BlockchainConfig,
LockingContractAddress, ChannelHubAddress, defaultBlockStep).
In `@pkg/blockchain/evm/blockchain_client.go`:
- Line 24: Rename the ambiguous constructor parameter name `contractAddress` to
`channelHubContractAddress` across the public constructor and all usages so it
matches the struct field `channelHubContractAddress` and avoids confusion with
other contracts; update the constructor signature (e.g., NewBlockchainClient),
its parameter list, and all references/pass-throughs at the call sites and
assignments (lines around where `channelHubContractAddress` is set and where
`contractAddress` was used, including the places mentioned at 36-37 and 57) to
use `channelHubContractAddress` consistently.
In `@pkg/core/interface.go`:
- Line 13: Add backward-compatible type aliases for the renamed exported
interfaces: declare `type Client = BlockchainClient` and `type
BlockchainEventHandler = ChannelHubEventHandler` alongside the new interface
declarations so external packages won't break; include brief deprecation
comments on these aliases noting they will be removed in the next major release
and ensure the aliases are exported (capitalized) and placed in the same package
near the original `BlockchainClient` and `ChannelHubEventHandler` declarations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb2f9573-a467-4b39-a341-286090c7c1a5
📒 Files selected for processing (36)
clearnode/blockchain_worker.goclearnode/chart/config/v1-rc/assets.yamlclearnode/chart/config/v1-rc/blockchains.yamlclearnode/event_handlers/interface.goclearnode/event_handlers/service.goclearnode/event_handlers/service_test.goclearnode/event_handlers/testing.goclearnode/main.goclearnode/store/database/database.goclearnode/store/memory/blockchain_config.goclearnode/store/memory/blockchain_config_test.goclearnode/store/memory/memory_store.gopkg/blockchain/evm/app_registry_abi.gopkg/blockchain/evm/app_registry_reactor.gopkg/blockchain/evm/app_registry_reactor_test.gopkg/blockchain/evm/base_client.gopkg/blockchain/evm/blockchain_client.gopkg/blockchain/evm/blockchain_test.gopkg/blockchain/evm/channel_hub_abi.gopkg/blockchain/evm/channel_hub_reactor.gopkg/blockchain/evm/client_opts.gopkg/blockchain/evm/erc20_abi.gopkg/blockchain/evm/init.gopkg/blockchain/evm/locking_client.gopkg/core/event.gopkg/core/interface.gopkg/core/types.gopkg/rpc/types.gosdk/go/client.gosdk/ts/src/blockchain/evm/app_registry_abi.tssdk/ts/src/blockchain/evm/index.tssdk/ts/src/blockchain/evm/locking_client.tssdk/ts/src/client.tssdk/ts/src/core/types.tssdk/ts/src/rpc/types.tssdk/ts/src/utils.ts
dimast-x
force-pushed
the
feat/integrate-locking
branch
4 times, most recently
from
2d547b4 to
41c5dd5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/blockchain/evm/channel_hub_reactor.go (3)
61-63:⚠️ Potential issue | 🔴 CriticalUse
channelHubEventMappinghere.
eventMappingis the package-global map populated bylocking_reactor.go, so ChannelHub logs are looked up in the wrong ABI table. In practice this makes ChannelHub events fall into the "unknown event ID" path unless a hash happens to collide.🛠️ Minimal fix
- eventName, ok := eventMapping[eventID] + eventName, ok := channelHubEventMapping[eventID]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/blockchain/evm/channel_hub_reactor.go` around lines 61 - 63, The code in channel_hub_reactor.go is using the wrong ABI map: replace the lookup that uses the package-global eventMapping with channelHubEventMapping so ChannelHub logs resolve to the correct ABI table; specifically, where eventID := l.Topics[0] and eventName, ok := eventMapping[eventID] are used, change the lookup to eventName, ok := channelHubEventMapping[eventID] (preserving the existing error/unknown-event handling).
368-443:⚠️ Potential issue | 🟠 MajorThese placeholder handlers currently acknowledge events as success.
handleMigrationInFinalized,handleMigrationOutInitiated,handleMigrationOutFinalized,handleDeposited,handleWithdrawn, andhandleEscrowDepositsPurgedonly log and returnnil. BecauseHandleEventpersists successful events, any occurrence of these topics is permanently consumed without updating state. Until core supports them, return an error or leave them out of dispatch.I can help sketch the missing core events and handler wiring if you want to split that into a follow-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/blockchain/evm/channel_hub_reactor.go` around lines 368 - 443, The listed handlers (handleMigrationInFinalized, handleMigrationOutInitiated, handleMigrationOutFinalized, handleDeposited, handleWithdrawn, handleEscrowDepositsPurged) currently only log and return nil which causes HandleEvent to persist unsupported events; change each to return a non-nil error indicating the event is not yet supported (e.g. errors.New or errors.Wrapf with context such as "unsupported blockchain event: MigrationInFinalized") so the event dispatcher won't mark them consumed, or remove these handlers from the dispatch list until core implements them; alternatively, if you prefer to implement them now, add the corresponding methods to core.BlockchainEventHandler and wire real handling logic instead of leaving these stub functions.
121-139:⚠️ Potential issue | 🔴 CriticalDon't persist failed ChannelHub events.
When a handler returns an error, this code still stores the log and emits
"processed event". The listener resume point comes frombb.DbStore.GetLatestEvent, so a transient handler or DB failure becomes a permanent skip on restart.🛠️ Minimal fix
if r.onEventProcessed != nil { r.onEventProcessed(r.blockchainID, err == nil) } if err != nil { logger.Warn("error processing event", "error", err) + return } if err := r.storeContractEvent(core.BlockchainEvent{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/blockchain/evm/channel_hub_reactor.go` around lines 121 - 139, The code currently stores the contract event via r.storeContractEvent and emits logger.Info("processed event", ...) even when the event handler returned an error; change the logic so that storing the event and emitting the "processed event" log only occur when the handler succeeded (err == nil). Specifically, after the handler completes, keep calling r.onEventProcessed(r.blockchainID, err == nil) as before but wrap the r.storeContractEvent(...) call and the logger.Info(...) call in a conditional (if err == nil) so failed events are not persisted or marked as processed.sdk/go/client.go (1)
329-331:⚠️ Potential issue | 🟠 MajorUse
DialContextso initialization respectsctx.Both
initializeBlockchainClientandinitializeLockingClientaccept a caller context but useethclient.Dial, ignoring cancellation and deadlines during RPC connection. Switching toethclient.DialContext(ctx, rpcURL)ensures these APIs remain responsive under network stalls or when the caller sets timeouts.Affected locations
- Lines 329-331 in
initializeBlockchainClient- Lines 434-436 in
initializeLockingClient🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/go/client.go` around lines 329 - 331, The connection code currently calls ethclient.Dial which ignores the passed ctx; update both initializeBlockchainClient and initializeLockingClient to call ethclient.DialContext(ctx, rpcURL) so the RPC dial respects cancellation and timeouts from the caller context, and propagate/handle the returned error exactly as before (return fmt.Errorf("failed to connect to blockchain RPC: %w", err) / similar in the locking initializer).
♻️ Duplicate comments (4)
pkg/blockchain/evm/locking_client.go (2)
92-93:⚠️ Potential issue | 🔴 CriticalReject malformed addresses before
HexToAddress.
common.HexToAddressdoes not fail on bad input; it pads, truncates, or partially decodes into another address. A typo intargetWalletAddress,destination, orusercan send funds or queries to the wrong account instead of returning an error.🛠️ Minimal fix
+func parseHexAddress(raw string) (common.Address, error) { + if !common.IsHexAddress(raw) { + return common.Address{}, errors.Errorf("invalid address %q", raw) + } + return common.HexToAddress(raw), nil +} + func (c *LockingClient) Lock(targetWalletAddress string, amount decimal.Decimal) (string, error) { - targetAddr := common.HexToAddress(targetWalletAddress) + targetAddr, err := parseHexAddress(targetWalletAddress) + if err != nil { + return "", err + } if c.transactOpts == nil { return "", errors.New("transaction signer not configured") } @@ func (c *LockingClient) Withdraw(destination string) (string, error) { if c.transactOpts == nil { return "", errors.New("transaction signer not configured") } - destinationAddr := common.HexToAddress(destination) + destinationAddr, err := parseHexAddress(destination) + if err != nil { + return "", err + } @@ func (c *LockingClient) GetBalance(user string) (decimal.Decimal, error) { - userAddr := common.HexToAddress(user) + userAddr, err := parseHexAddress(user) + if err != nil { + return decimal.Zero, err + } lockingContract, err := NewAppRegistry(c.lockingContractAddress, c.evmClient)What does go-ethereum common.HexToAddress do with malformed input, and should callers validate with common.IsHexAddress first?Also applies to: 151-156, 202-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/blockchain/evm/locking_client.go` around lines 92 - 93, The Lock function currently calls common.HexToAddress on targetWalletAddress without validation which can silently accept malformed input; before calling common.HexToAddress in Lock (and the other address-converting sites noted), validate the input with common.IsHexAddress(targetWalletAddress) and return a clear error when it returns false, otherwise proceed to convert and use the resulting address; update any similar functions that convert user-provided hex addresses (the other address conversion sites flagged) to follow the same pattern so typos do not become silently padded/truncated addresses.
98-99:⚠️ Potential issue | 🟠 MajorFail fast on unsupported precision and negative amounts.
decimalToBigInt()callsDecimal.BigInt(), which truncates toward zero. Values with more fractional digits than the token supports can silently round down to a smaller amount, and negative values currently pass through.🛠️ Minimal fix
- amountBig := decimalToBigInt(amount, int32(c.tokenDecimals)) + amountBig, err := decimalToBigInt(amount, int32(c.tokenDecimals)) + if err != nil { + return "", errors.Wrap(err, "invalid amount") + } @@ - amountBig := decimalToBigInt(amount, int32(c.tokenDecimals)) + amountBig, err := decimalToBigInt(amount, int32(c.tokenDecimals)) + if err != nil { + return "", errors.Wrap(err, "invalid amount") + } @@ -func decimalToBigInt(amount decimal.Decimal, decimals int32) *big.Int { +func decimalToBigInt(amount decimal.Decimal, decimals int32) (*big.Int, error) { + if amount.IsNegative() { + return nil, errors.New("amount must be non-negative") + } shifted := amount.Shift(decimals) - return shifted.BigInt() + if !shifted.Equal(shifted.Truncate(0)) { + return nil, errors.New("amount has more precision than the token supports") + } + return shifted.BigInt(), nil }Does github.com/shopspring/decimal v1.4.0 Decimal.BigInt() truncate fractional digits instead of rounding?Also applies to: 177-177, 217-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/blockchain/evm/locking_client.go` around lines 98 - 99, decimalToBigInt currently relies on Decimal.BigInt which truncates and allows negatives; update validation so callers (or decimalToBigInt itself) reject negative amounts and any Decimal with more fractional digits than int32(c.tokenDecimals) before converting. Specifically, in places using decimalToBigInt (e.g., the call where amountBig := decimalToBigInt(amount, int32(c.tokenDecimals)) and the other locations noted), check amount.Sign() >= 0 and compute the number of fractional digits on the Decimal (using shopspring/decimal APIs) and return an error if fractional digits > tokenDecimals; alternatively move these checks into decimalToBigInt so it returns an error for negatives or unsupported precision instead of silently truncating. Ensure error messages include the offending value and required precision.pkg/blockchain/evm/blockchain_client.go (1)
234-240:⚠️ Potential issue | 🟠 MajorReturn the node address in escrow responses.
GetHomeChannelData()usesNodefor the counterparty node address, but these two responses now populate it withc.channelHubContractAddress. Callers reusing the same field semantics will receive the contract address instead of the node. Usec.nodeAddress.Hex()here, or rename the response field if the contract address is intentional.🛠️ Minimal fix
return core.EscrowDepositDataResponse{ EscrowChannelID: escrowChannelID, - Node: c.channelHubContractAddress.Hex(), + Node: c.nodeAddress.Hex(), LastState: *lastState, UnlockExpiry: data.UnlockAt, ChallengeExpiry: data.ChallengeExpiry, }, nil @@ return core.EscrowWithdrawalDataResponse{ EscrowChannelID: escrowChannelID, - Node: c.channelHubContractAddress.Hex(), + Node: c.nodeAddress.Hex(), LastState: *lastState, }, nilAlso applies to: 259-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/blockchain/evm/blockchain_client.go` around lines 234 - 240, The responses for escrow operations are populating the Node field with c.channelHubContractAddress.Hex() instead of the counterparty node; change the Node assignment to use c.nodeAddress.Hex() in the functions that return core.EscrowDepositDataResponse (the returned struct shown) and the other similar response block later (the second occurrence around lines 259-263) so callers get the node address; if the contract address was intentional, rename the response field instead.pkg/blockchain/evm/locking_reactor.go (1)
89-107:⚠️ Potential issue | 🔴 CriticalDon't advance the locking-event cursor after a handler failure.
This still stores the log and logs
"processed event"even whenhandleLocked/handleWithdrawnreturned an error. Since the listener resumes from the last stored event, failed logs will be skipped forever after restart.🛠️ Minimal fix
if r.onEventProcessed != nil { r.onEventProcessed(r.blockchainID, err == nil) } if err != nil { logger.Warn("error processing event", "error", err) + return } if err := r.storeContractEvent(core.BlockchainEvent{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/blockchain/evm/locking_reactor.go` around lines 89 - 107, The code advances the locking-event cursor by calling r.storeContractEvent and logging "processed event" even when the handlers (handleLocked/handleWithdrawn) returned an error; move the call to r.storeContractEvent(...) and the logger.Info("processed event", ...) so they only run when err == nil (i.e., inside the success branch), keeping the existing r.onEventProcessed(r.blockchainID, err == nil) behavior so failures are signaled but failed logs are not stored/marked processed; update references around r.storeContractEvent, logger.Info and the error check to ensure failed handler runs do not persist the event.
🧹 Nitpick comments (3)
pkg/blockchain/evm/blockchain_test.go (1)
62-63: Update stale comment.The comment references
NewClient/NewChannelHubbut the code now usesNewBlockchainClient. Update the comment to reflect the renamed constructor.Proposed fix
- // Mock models simulate an EVM client where NewClient/NewChannelHub return a local struct without external calls + // Mock models simulate an EVM client where NewBlockchainClient returns a local struct without external calls🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/blockchain/evm/blockchain_test.go` around lines 62 - 63, Update the stale test comment that still references NewClient/NewChannelHub to reflect the current constructor name NewBlockchainClient; locate the comment above the client creation in blockchain_test.go and change the text to mention NewBlockchainClient (and any other renamed symbols if applicable) so the comment accurately describes that the mock models simulate an EVM client and that NewBlockchainClient returns a local struct without external calls.pkg/blockchain/evm/locking_reactor_test.go (1)
61-64: Consider tracking callback invocation in the success path too.While less critical than the error path, tracking that the callback was actually invoked would make the test more robust.
Proposed improvement
- var processedSuccess bool + callbackInvoked := false + processedSuccess := false reactor.SetOnEventProcessed(func(_ uint64, success bool) { + callbackInvoked = true processedSuccess = success }) reactor.HandleEvent(context.Background(), logEntry) + assert.True(t, callbackInvoked, "callback should have been invoked")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/blockchain/evm/locking_reactor_test.go` around lines 61 - 64, The test currently records only the success value from the reactor.SetOnEventProcessed callback (processedSuccess) but doesn't assert that the callback was actually invoked; add a separate flag (e.g., processedInvoked) and set it inside the same callback alongside processedSuccess, then update the test assertions to require processedInvoked == true and processedSuccess == true so the test verifies both invocation and success for the success path. Ensure you reference the existing reactor.SetOnEventProcessed callback and processedSuccess variable when adding the new processedInvoked flag and assertions.clearnode/store/memory/blockchain_config.go (1)
22-27: Update stale struct comment.The comment mentions "default contract addresses that apply to all blockchains unless overridden" but the
DefaultContractAddressesfield and associated logic have been removed. Update the comment to reflect the current structure.Proposed fix
-// BlockchainsConfig represents the root configuration structure for all blockchain settings. -// It contains default contract addresses that apply to all blockchains unless overridden, -// and a list of individual blockchain configurations. +// BlockchainsConfig represents the root configuration structure for all blockchain settings. +// It contains a list of individual blockchain configurations. type BlockchainsConfig struct { Blockchains []BlockchainConfig `yaml:"blockchains"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/memory/blockchain_config.go` around lines 22 - 27, The struct comment for BlockchainsConfig is stale — remove the reference to "default contract addresses" and replace it with a short accurate description stating that BlockchainsConfig is the root configuration for blockchain settings and contains a list/slice of per-blockchain configurations (Blockchains []BlockchainConfig `yaml:"blockchains"`); update the doc comment above type BlockchainsConfig to reflect only the current structure and purpose.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clearnode/main.go`:
- Around line 98-110: The listeners/workers (evm.NewListener -> l.Listen and
NewBlockchainWorker -> worker.Start) are started with an uncancelled root ctx so
they may access bb.DbStore after bb.Close(); change to create a cancelable child
context for these goroutines (e.g., ctx, cancel := context.WithCancel(rootCtx)),
store the cancel function, signal cancel during shutdown, and wait for the
listener/worker to exit (use their provided stop callbacks, a sync.WaitGroup, or
a done channel) before calling bb.Close(); ensure l.Listen and worker.Start
receive the cancellable ctx and that shutdown invokes cancel and waits for those
goroutines to finish to avoid races with StoreContractEvent/GetLatestEvent/RPC
calls.
In `@clearnode/store/memory/memory_store.go`:
- Around line 31-37: The runtime currently calls checkChannelHubVersion for
every core.Blockchain even when Blockchain.ChannelHubAddress is empty, which
leads to common.HexToAddress("") and contract checks against the zero address;
update the runtime loop that invokes checkChannelHubVersion to first guard on
blockchain.ChannelHubAddress being non-empty (mirror the pattern used in main.go
where ChannelHubAddress is checked before use) and skip calling
checkChannelHubVersion for blockchains with an empty ChannelHubAddress so only
valid addresses are passed to checkChannelHubVersion.
In `@pkg/blockchain/evm/init.go`:
- Around line 3-6: The code in init() uses the wrong event map variable when
resolving ChannelHub events: replace the lookup that uses eventMapping with
channelHubEventMapping so ChannelHub event IDs map correctly; locate the lookup
statement (currently doing eventName, ok := eventMapping[eventID]) and change it
to use channelHubEventMapping, ensuring initChannelHub()’s initialization is
actually used rather than the locking-contract eventMapping.
In `@pkg/blockchain/evm/locking_client_test.go`:
- Around line 11-29: TestLockingClientIntegration currently contains a hardcoded
Alchemy RPC URL and doesn't honor the RPC_URL env var or skip when not provided;
update the test to read rpcURL from os.Getenv("RPC_URL"), remove the hardcoded
key, and if the env var is empty call t.Skip("integration test skipped: RPC_URL
not set"); keep the rest of the flow (ethclient.Dial, NewLockingClient,
assertions) unchanged and add the "os" import to enable getenv/skip behavior.
In `@pkg/blockchain/evm/locking_reactor_test.go`:
- Around line 68-72: The test currently normalizes capturedEvent.UserAddress via
common.HexToAddress(...).String(), which masks the reactor's lowercasing; change
the assertion to compare capturedEvent.UserAddress directly to the expected
lowercase address (e.g., strings.ToLower(userAddr.String())) to ensure
locking_reactor.go's lowercasing at the UserAddress assignment is enforced, and
add any necessary import for strings in the test file; keep other assertions
unchanged (refer to capturedEvent.UserAddress and the lowercasing behavior
implemented in locking_reactor.go:118).
In `@pkg/rpc/types.go`:
- Around line 286-287: The field LockingContractAddress currently has a
misleading comment referencing the "app registry" and is a non-optional string;
update the comment to accurately describe that this field is the contract
address for the locking contract on the network (e.g., "LockingContractAddress
is the contract address for the locking contract on this network") and change
the field type to a pointer (e.g., *string) with the json tag
`json:"locking_contract_address,omitempty"` so it becomes optional and will be
omitted when not set, aligning with the TypeScript SDK's optional
`locking_contract_address`.
In `@sdk/go/client.go`:
- Around line 51-60: The blockchainClients and blockchainLockingClients maps are
accessed concurrently and need synchronization: add a mutex field (e.g.,
chainsMu sync.Mutex) to the client struct alongside blockchainClients and
blockchainLockingClients, and use it to guard all reads and writes to those maps
(wrap lookups, initializations, and assignments) in Checkpoint,
EscrowSecurityTokens, GetLockedBalance and the other affected methods (the
ranges you noted). Alternatively implement a per-chain sync.Once keyed by chain
ID to ensure single initialization; in either case ensure you hold the mutex (or
use the per-chain once) around the fast-path check and the initialization to
prevent double-initialize and concurrent map writes.
---
Outside diff comments:
In `@pkg/blockchain/evm/channel_hub_reactor.go`:
- Around line 61-63: The code in channel_hub_reactor.go is using the wrong ABI
map: replace the lookup that uses the package-global eventMapping with
channelHubEventMapping so ChannelHub logs resolve to the correct ABI table;
specifically, where eventID := l.Topics[0] and eventName, ok :=
eventMapping[eventID] are used, change the lookup to eventName, ok :=
channelHubEventMapping[eventID] (preserving the existing error/unknown-event
handling).
- Around line 368-443: The listed handlers (handleMigrationInFinalized,
handleMigrationOutInitiated, handleMigrationOutFinalized, handleDeposited,
handleWithdrawn, handleEscrowDepositsPurged) currently only log and return nil
which causes HandleEvent to persist unsupported events; change each to return a
non-nil error indicating the event is not yet supported (e.g. errors.New or
errors.Wrapf with context such as "unsupported blockchain event:
MigrationInFinalized") so the event dispatcher won't mark them consumed, or
remove these handlers from the dispatch list until core implements them;
alternatively, if you prefer to implement them now, add the corresponding
methods to core.BlockchainEventHandler and wire real handling logic instead of
leaving these stub functions.
- Around line 121-139: The code currently stores the contract event via
r.storeContractEvent and emits logger.Info("processed event", ...) even when the
event handler returned an error; change the logic so that storing the event and
emitting the "processed event" log only occur when the handler succeeded (err ==
nil). Specifically, after the handler completes, keep calling
r.onEventProcessed(r.blockchainID, err == nil) as before but wrap the
r.storeContractEvent(...) call and the logger.Info(...) call in a conditional
(if err == nil) so failed events are not persisted or marked as processed.
In `@sdk/go/client.go`:
- Around line 329-331: The connection code currently calls ethclient.Dial which
ignores the passed ctx; update both initializeBlockchainClient and
initializeLockingClient to call ethclient.DialContext(ctx, rpcURL) so the RPC
dial respects cancellation and timeouts from the caller context, and
propagate/handle the returned error exactly as before (return fmt.Errorf("failed
to connect to blockchain RPC: %w", err) / similar in the locking initializer).
---
Duplicate comments:
In `@pkg/blockchain/evm/blockchain_client.go`:
- Around line 234-240: The responses for escrow operations are populating the
Node field with c.channelHubContractAddress.Hex() instead of the counterparty
node; change the Node assignment to use c.nodeAddress.Hex() in the functions
that return core.EscrowDepositDataResponse (the returned struct shown) and the
other similar response block later (the second occurrence around lines 259-263)
so callers get the node address; if the contract address was intentional, rename
the response field instead.
In `@pkg/blockchain/evm/locking_client.go`:
- Around line 92-93: The Lock function currently calls common.HexToAddress on
targetWalletAddress without validation which can silently accept malformed
input; before calling common.HexToAddress in Lock (and the other
address-converting sites noted), validate the input with
common.IsHexAddress(targetWalletAddress) and return a clear error when it
returns false, otherwise proceed to convert and use the resulting address;
update any similar functions that convert user-provided hex addresses (the other
address conversion sites flagged) to follow the same pattern so typos do not
become silently padded/truncated addresses.
- Around line 98-99: decimalToBigInt currently relies on Decimal.BigInt which
truncates and allows negatives; update validation so callers (or decimalToBigInt
itself) reject negative amounts and any Decimal with more fractional digits than
int32(c.tokenDecimals) before converting. Specifically, in places using
decimalToBigInt (e.g., the call where amountBig := decimalToBigInt(amount,
int32(c.tokenDecimals)) and the other locations noted), check amount.Sign() >= 0
and compute the number of fractional digits on the Decimal (using
shopspring/decimal APIs) and return an error if fractional digits >
tokenDecimals; alternatively move these checks into decimalToBigInt so it
returns an error for negatives or unsupported precision instead of silently
truncating. Ensure error messages include the offending value and required
precision.
In `@pkg/blockchain/evm/locking_reactor.go`:
- Around line 89-107: The code advances the locking-event cursor by calling
r.storeContractEvent and logging "processed event" even when the handlers
(handleLocked/handleWithdrawn) returned an error; move the call to
r.storeContractEvent(...) and the logger.Info("processed event", ...) so they
only run when err == nil (i.e., inside the success branch), keeping the existing
r.onEventProcessed(r.blockchainID, err == nil) behavior so failures are signaled
but failed logs are not stored/marked processed; update references around
r.storeContractEvent, logger.Info and the error check to ensure failed handler
runs do not persist the event.
---
Nitpick comments:
In `@clearnode/store/memory/blockchain_config.go`:
- Around line 22-27: The struct comment for BlockchainsConfig is stale — remove
the reference to "default contract addresses" and replace it with a short
accurate description stating that BlockchainsConfig is the root configuration
for blockchain settings and contains a list/slice of per-blockchain
configurations (Blockchains []BlockchainConfig `yaml:"blockchains"`); update the
doc comment above type BlockchainsConfig to reflect only the current structure
and purpose.
In `@pkg/blockchain/evm/blockchain_test.go`:
- Around line 62-63: Update the stale test comment that still references
NewClient/NewChannelHub to reflect the current constructor name
NewBlockchainClient; locate the comment above the client creation in
blockchain_test.go and change the text to mention NewBlockchainClient (and any
other renamed symbols if applicable) so the comment accurately describes that
the mock models simulate an EVM client and that NewBlockchainClient returns a
local struct without external calls.
In `@pkg/blockchain/evm/locking_reactor_test.go`:
- Around line 61-64: The test currently records only the success value from the
reactor.SetOnEventProcessed callback (processedSuccess) but doesn't assert that
the callback was actually invoked; add a separate flag (e.g., processedInvoked)
and set it inside the same callback alongside processedSuccess, then update the
test assertions to require processedInvoked == true and processedSuccess == true
so the test verifies both invocation and success for the success path. Ensure
you reference the existing reactor.SetOnEventProcessed callback and
processedSuccess variable when adding the new processedInvoked flag and
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2257d200-eb66-49c6-b8dd-802cdb890078
📒 Files selected for processing (39)
clearnode/api/node_v1/utils.goclearnode/blockchain_worker.goclearnode/chart/config/v1-rc/assets.yamlclearnode/chart/config/v1-rc/blockchains.yamlclearnode/event_handlers/interface.goclearnode/event_handlers/service.goclearnode/event_handlers/service_test.goclearnode/event_handlers/testing.goclearnode/main.goclearnode/store/database/database.goclearnode/store/memory/blockchain_config.goclearnode/store/memory/blockchain_config_test.goclearnode/store/memory/memory_store.gopkg/blockchain/evm/app_registry_abi.gopkg/blockchain/evm/base_client.gopkg/blockchain/evm/blockchain_client.gopkg/blockchain/evm/blockchain_test.gopkg/blockchain/evm/channel_hub_abi.gopkg/blockchain/evm/channel_hub_reactor.gopkg/blockchain/evm/client_opts.gopkg/blockchain/evm/erc20_abi.gopkg/blockchain/evm/init.gopkg/blockchain/evm/locking_client.gopkg/blockchain/evm/locking_client_test.gopkg/blockchain/evm/locking_reactor.gopkg/blockchain/evm/locking_reactor_test.gopkg/core/event.gopkg/core/interface.gopkg/core/types.gopkg/rpc/types.gosdk/go/client.gosdk/go/utils.gosdk/ts/src/blockchain/evm/app_registry_abi.tssdk/ts/src/blockchain/evm/index.tssdk/ts/src/blockchain/evm/locking_client.tssdk/ts/src/client.tssdk/ts/src/core/types.tssdk/ts/src/rpc/types.tssdk/ts/src/utils.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- sdk/ts/src/blockchain/evm/index.ts
- pkg/core/types.go
- pkg/blockchain/evm/client_opts.go
- sdk/ts/src/rpc/types.ts
- pkg/core/event.go
- clearnode/event_handlers/interface.go
- sdk/ts/src/blockchain/evm/locking_client.ts
- clearnode/store/database/database.go
- clearnode/chart/config/v1-rc/assets.yaml
- clearnode/event_handlers/testing.go
- sdk/ts/src/utils.ts
- pkg/blockchain/evm/base_client.go
- sdk/ts/src/blockchain/evm/app_registry_abi.ts
dimast-x
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cerebro/operator.go (1)
206-206: Minor formatting inconsistency.The
exitcommand entry has different indentation compared to other entries in the suggestions list.🔧 Suggested fix
-{Text: "exit", Description: "Exit the CLI"}, + {Text: "exit", Description: "Exit the CLI"},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cerebro/operator.go` at line 206, The suggestions list contains an entry `{Text: "exit", Description: "Exit the CLI"}` that is indented differently than the other entries; locate the suggestions slice/array (where entries like `{Text: "...", Description: "..."}` are listed) and adjust the indentation of the `Text: "exit"` entry to match the surrounding entries so formatting is consistent (preserve the exact field order and spacing style used by the other entries).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cerebro/operator.go`:
- Line 206: The suggestions list contains an entry `{Text: "exit", Description:
"Exit the CLI"}` that is indented differently than the other entries; locate the
suggestions slice/array (where entries like `{Text: "...", Description: "..."}`
are listed) and adjust the indentation of the `Text: "exit"` entry to match the
surrounding entries so formatting is consistent (preserve the exact field order
and spacing style used by the other entries).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e437a05d-edbd-43bd-8a7a-1a38d9d813fb
📒 Files selected for processing (5)
cerebro/commands.gocerebro/main.gocerebro/operator.gocerebro/operator_test.gocerebro/storage.go
alessio
approved these changes
Mar 7, 2026
Co-authored-by: Anton Filonenko <philanton@users.noreply.github.com>
dimast-x
force-pushed
the
feat/integrate-locking
branch
from
8ba6337 to
15fa103
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sdk/ts/src/core/types.ts (1)
164-169:⚠️ Potential issue | 🟠 MajorMake
channelHubAddressoptional too.The backend validates that blockchains must have at least one of
channel_hub_addressorlocking_contract_address, allowing lock-only blockchains without a channel hub. However, the TypeScript types still require both fields, making it impossible to represent a valid lock-only blockchain without an unsafe cast. The transformer inutils.tsline 43 blindly castsinfo.channel_hub_address as Address, which could be an empty string from the backend.While defensive checks exist in
client.ts(lines 1781, 1810), the type system should accurately reflect what the backend allows. Please makechannelHubAddressoptional to match the backend schema.💡 Suggested fix
export interface Blockchain { name: string; id: bigint; // uint64 - channelHubAddress: Address; + channelHubAddress?: Address; lockingContractAddress?: Address; blockStep: bigint; // uint64 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts/src/core/types.ts` around lines 164 - 169, Update the Blockchain interface so channelHubAddress is optional (change channelHubAddress: Address to channelHubAddress?: Address) to match backend semantics, and adjust the transformer that maps info.channel_hub_address (the cast in utils.ts currently doing info.channel_hub_address as Address) to defensively handle empty string/undefined (e.g., check truthiness and set undefined or a valid Address accordingly) so code consuming Blockchain (and the existing lockingContractAddress checks) type-checks correctly without unsafe casts.pkg/blockchain/evm/channel_hub_reactor.go (1)
103-139:⚠️ Potential issue | 🟠 MajorDon't acknowledge TODO-only events as processed.
These handlers only log and return
nil. BecauseHandleEvent()persists any nil result,MigrationInFinalized,MigrationOutInitiated,MigrationOutFinalized,Deposited,Withdrawn, andEscrowDepositsPurgedwill advance the resume cursor without updating any state, so a later real implementation won't be able to replay them. Either wire real handlers before routing these cases, or keep them out of the persisted-success path.Also applies to: 371-446
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/blockchain/evm/channel_hub_reactor.go` around lines 103 - 139, The switch currently calls stub handlers like handleMigrationInFinalized, handleMigrationOutInitiated, handleMigrationOutFinalized, handleDeposited, handleWithdrawn, and handleEscrowDepositsPurged which only log and return nil, causing HandleEvent() to persist them as processed; change this so TODO-only handlers do not mark events as successfully handled: either implement the real handler logic, or have each stub return a sentinel error (e.g. ErrUnimplemented) and update the calling code around the switch (the block that calls r.onEventProcessed(...) and r.storeContractEvent(...)) to treat ErrUnimplemented as a non-success that skips onEventProcessed/storeContractEvent (and log accordingly). Reference the functions handleMigrationInFinalized, handleMigrationOutInitiated, handleMigrationOutFinalized, handleDeposited, handleWithdrawn, handleEscrowDepositsPurged and the post-switch persistence logic to locate where to change behavior.
♻️ Duplicate comments (3)
clearnode/store/memory/blockchain_config.go (1)
86-88:⚠️ Potential issue | 🔴 CriticalLock-only configs still conflict with clearnode startup.
verifyBlockchainsConfignow accepts a blockchain withoutchannel_hub_address, butclearnode/runtime.goLines 220-248 still verifies ChannelHub on every blockchain returned byGetBlockchains(). Until that runtime guard lands, this change still permits configs that crash startup. Either keep the stricter validation here or merge the runtime fix in the same PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/memory/blockchain_config.go` around lines 86 - 88, The new validation allows blockchains without ChannelHubAddress but runtime.go still assumes every blockchain has a ChannelHub and crashes; revert to the stricter check in verifyBlockchainsConfig so configs must include channel_hub_address (i.e., require bc.ChannelHubAddress != ""), or alternatively update the startup logic in runtime.go's ChannelHub verification loop (the code iterating GetBlockchains()) to skip ChannelHub checks for entries where bc.ChannelHubAddress is empty and only validate locking_contract_address where provided; reference verifyBlockchainsConfig, GetBlockchains, ChannelHubAddress, LockingContractAddress and the ChannelHub verification block in runtime.go when making the change.clearnode/main.go (1)
31-33:⚠️ Potential issue | 🟠 MajorCancellation still races with teardown.
cancelBlockchain()only signals the listeners/workers;bb.Close()runs immediately afterward, andctxis stillcontext.Background()forrunStoreMetricsExporter()at Line 138. That leaves in-flight listeners, workers, and the metrics exporter free to hitbb.DbStorewhile shutdown is closing it. Use a cancelable root context for all background goroutines and wait for them to exit before closing the backbone.Also applies to: 101-112, 128-135, 188-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/main.go` around lines 31 - 33, rootCtx is created as a non-cancelable context which causes a race where cancelBlockchain() signals workers but runStoreMetricsExporter and other goroutines still use context.Background() and may access bb.DbStore while bb.Close() runs; change rootCtx to a cancelable context (use context.WithCancel), propagate that cancelable ctx (the returned ctx variable) into runStoreMetricsExporter and all background goroutines instead of context.Background(), call the cancel function before bb.Close(), and ensure you wait for those goroutines to exit (e.g., sync.WaitGroup or channel coordination used by the listeners/workers referenced in blockchainCtx/cancelBlockchain and runStoreMetricsExporter) before calling bb.Close() to avoid accesses to bb.DbStore during teardown; apply the same fix where rootCtx/ctx/blockchainCtx are created and used (rootCtx, blockchainCtx, cancelBlockchain, ctx, runStoreMetricsExporter, bb.Close, bb.DbStore).pkg/blockchain/evm/blockchain_client.go (1)
234-240:⚠️ Potential issue | 🟠 MajorReturn the counterparty node here, not the ChannelHub contract.
GetHomeChannelData()still usesNodefor the participant node address, but these escrow responses now fill the same field withc.channelHubContractAddress.Hex(). Callers that treatNodeas the counterparty will get the contract address instead.Minimal fix
return core.EscrowDepositDataResponse{ EscrowChannelID: escrowChannelID, - Node: c.channelHubContractAddress.Hex(), + Node: c.nodeAddress.Hex(), LastState: *lastState, UnlockExpiry: data.UnlockAt, ChallengeExpiry: data.ChallengeExpiry, }, nil @@ return core.EscrowWithdrawalDataResponse{ EscrowChannelID: escrowChannelID, - Node: c.channelHubContractAddress.Hex(), + Node: c.nodeAddress.Hex(), LastState: *lastState, }, nilAlso applies to: 259-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/blockchain/evm/blockchain_client.go` around lines 234 - 240, The response is incorrectly setting EscrowDepositDataResponse.Node to the ChannelHub contract address (c.channelHubContractAddress.Hex()); change it to return the counterparty/participant node address instead (the same value GetHomeChannelData() expects in Node). Locate where EscrowDepositDataResponse is constructed (the block that returns EscrowDepositDataResponse with EscrowChannelID, Node, LastState, UnlockExpiry, ChallengeExpiry) and replace Node: c.channelHubContractAddress.Hex() with the counterparty address from the channel/lastState/escrow data (e.g., use the participant/counterparty field available on lastState or the escrow data), and make the same fix in the analogous return at lines 259-263. Ensure the Node field contains the peer node's address string, not the contract address.
🧹 Nitpick comments (6)
cerebro/commands.go (2)
1290-1307: Consider validating the destination address before calling the SDK.The
withdrawSecurityTokensfunction passes thedestinationaddress directly too.client.WithdrawSecurityTokenswithout validation. Based on theLockingClient.Withdrawimplementation (context snippet 3), invalid addresses are silently converted viacommon.HexToAddress, which could result in sending tokens to an unintended address (e.g., the zero address if the input is malformed).Adding client-side validation would provide better error messages to the user.
🛡️ Proposed validation
+import "github.com/ethereum/go-ethereum/common" + func (o *Operator) withdrawSecurityTokens(ctx context.Context, chainIDStr, destination string) { chainID, err := o.parseChainID(chainIDStr) if err != nil { fmt.Printf("ERROR: %v\n", err) return } + if !common.IsHexAddress(destination) { + fmt.Printf("ERROR: Invalid destination address: %s\n", destination) + return + } + fmt.Printf("Withdrawing security tokens to %s on chain %d...\n", destination, chainID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cerebro/commands.go` around lines 1290 - 1307, Validate the destination address in withdrawSecurityTokens before calling o.client.WithdrawSecurityTokens: use the same address validation utility the project uses (e.g., common.IsHexAddress or the repo's address validation helper) to reject malformed or empty inputs and print a clear error via fmt.Printf; keep parseChainID and the subsequent flow intact but return early on invalid destination so the SDK isn't called with a possibly zero/incorrect address.
1334-1350: Consider validating user-provided wallet address.When the user explicitly provides a wallet address (via
args[3]in the caller), it's passed toGetLockedBalancewithout validation. Per context snippet 1, the underlyingLockingClient.GetBalancedoes not validate addresses—it directly converts them withcommon.HexToAddress, which silently produces potentially incorrect addresses for malformed input.🛡️ Proposed validation
func (o *Operator) securityBalance(ctx context.Context, chainIDStr, wallet string) { chainID, err := o.parseChainID(chainIDStr) if err != nil { fmt.Printf("ERROR: %v\n", err) return } + if !common.IsHexAddress(wallet) { + fmt.Printf("ERROR: Invalid wallet address: %s\n", wallet) + return + } + fmt.Printf("Querying security token balance for %s on chain %d...\n", wallet, chainID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cerebro/commands.go` around lines 1334 - 1350, In securityBalance, validate the user-supplied wallet string before calling o.client.GetLockedBalance: ensure the wallet is non-empty and is a valid hex Ethereum address (e.g., use common.IsHexAddress or equivalent validation) and if invalid print an error and return; reference the securityBalance function and the caller that passes args[3] so the fix prevents malformed input reaching LockingClient.GetLockedBalance which uses common.HexToAddress silently.cerebro/operator.go (1)
206-206: Fix inconsistent indentation.Line 206 has inconsistent indentation compared to the surrounding code (missing leading tab).
🧹 Proposed fix
-{Text: "exit", Description: "Exit the CLI"}, + {Text: "exit", Description: "Exit the CLI"},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cerebro/operator.go` at line 206, The entry {Text: "exit", Description: "Exit the CLI"} has inconsistent indentation; align it with the surrounding slice/struct entries by adding the missing leading tab(s) so it matches the indentation style used by nearby entries (e.g., other command items in the same slice or block) inside operator.go—locate the block containing the command definitions (the literal with Text/Description pairs) and adjust the indentation of the "exit" line to be consistent.pkg/blockchain/evm/listener_test.go (1)
67-69: Add a regression test for callback failures.
HandleEventnow returns an error, but the updated tests still cover only thenilpath. Please add one case wherehandleEventreturns a sentinel error and assert thatListenforwards it tohandleClosure; otherwise the new control-flow branch is still untested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/blockchain/evm/listener_test.go` around lines 67 - 69, Add a regression test that verifies Listen forwards non-nil callback errors to handleClosure: define a sentinel error (e.g., var sentinel = errors.New("sentinel")), set handleEvent to return that sentinel instead of closing eventHandled, stub or capture handleClosure to receive errors, invoke Listen (using the same setup as the existing test), wait for handleClosure to be called, and assert the received error equals the sentinel; update the test case in listener_test.go to include this new branch so handleEvent -> Listen -> handleClosure error propagation is exercised.clearnode/store/memory/blockchain_config_test.go (1)
18-51: Please cover the new negative validation branches too.The table now proves a lock-only blockchain is accepted, but it still misses the two new failure paths in
verifyBlockchainsConfig: both addresses missing, and malformedlocking_contract_address. Those are the cases most likely to regress here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/memory/blockchain_config_test.go` around lines 18 - 51, Add unit tests in blockchain_config_test.go to cover the two negative branches of verifyBlockchainsConfig: (1) a BlockchainConfig that has neither ChannelHubAddress nor LockingContractAddress and should return an error, and (2) a BlockchainConfig with a malformed LockingContractAddress (invalid hex/length) which should also return an error; construct BlockchainsConfig test cases that set ID/Name and the relevant address fields, call the same validation path (verifyBlockchainsConfig or via the existing loader used in tests), assert that an error is returned and that the error message discriminates the case, and ensure other expectations like defaultBlockStep remain unchanged for non-error cases (use BlockchainsConfig, BlockchainConfig, verifyBlockchainsConfig, defaultBlockStep to locate code).pkg/core/interface.go (1)
50-61: RenameAppRegistryClientto match the locking terminology before release.The rest of the PR exposes this surface as
LockingClient,LockingContractReactor, andLockingContractAddress. KeepingAppRegistryClienthere bakes the old abstraction into the public core API and makes a later cleanup a breaking rename.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/core/interface.go` around lines 50 - 61, The interface name AppRegistryClient in this file should be renamed to LockingClient to match the rest of the PR and avoid a breaking public API rename later; update the type declaration `type AppRegistryClient interface { ... }` to `type LockingClient interface { ... }` and ensure any references in the codebase (including usages alongside LockingContractReactor and LockingContractAddress) are updated to the new symbol so the public core API consistently uses the "Locking" terminology.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cerebro/operator.go`:
- Around line 76-97: The code currently treats any error from
o.store.GetPrivateKey() as “no wallet” and auto-generates a new key; change the
logic to only auto-generate when GetPrivateKey returns the explicit sentinel “no
private key” condition (e.g., ErrNoPrivateKey or sql.ErrNoRows) and to propagate
any other errors up instead of replacing the wallet. Update the
store.GetPrivateKey implementation (or its callers) so it returns a well-known
sentinel error you can check with errors.Is(), then in the operator
initialization check errors.Is(err, store.ErrNoPrivateKey) before calling
generatePrivateKey(), otherwise return fmt.Errorf("failed to get private key:
%w", err); keep using SetPrivateKey, sign.NewEthereumRawSigner and the existing
welcome messaging only in the sentinel-case.
In `@clearnode/store/memory/memory_store.go`:
- Around line 27-29: The current gating for adding entries to
supportedBlockchainIDs only checks bc.ChannelHubAddress and thus excludes
lock-only chains; update the condition where supportedBlockchainIDs[bc.ID] is
set (in memory_store.go) to also include blockchains that define a locking
contract (e.g., check bc.LockingContractAddress or the struct field representing
locking_contract_address). In other words, change the if that references
bc.ChannelHubAddress to something like "if bc.ChannelHubAddress != "" ||
bc.LockingContractAddress != """ so lock-only chains are included for
supportedAssets, tokenDecimals, and SuggestedBlockchainID resolution (this
touches the code that builds supportedBlockchainIDs used later by
verifyBlockchainsConfig).
In `@pkg/blockchain/evm/init.go`:
- Around line 3-6: The package init currently calls initChannelHub() and
initLockingContract() which can panic on ABI parse failures; replace the eager
panic-only setup with a lazy, recoverable initializer: remove those calls from
init(), create a package-level initializer function (e.g., ensureEVMSetup() or
InitEVMOnce()) that uses sync.Once and returns an error, and have
initChannelHub() and initLockingContract() either become helper funcs invoked by
that initializer or return errors themselves so the initializer
aggregates/returns any parse errors; then call this initializer from the
appropriate constructors/startup path (e.g., the reactor or service
constructors) so callers can handle the error instead of crashing at import
time. Ensure the new initializer uses sync.Once to avoid repeated setup and
exposes an error to callers.
In `@pkg/blockchain/evm/listener.go`:
- Around line 90-93: The current Listen flow returns immediately when
getLatestEvent fails, which lets transient DB blips crash the process; instead,
wrap the call to getLatestEvent (the method named getLatestEvent used inside
Listen that references l.contractAddress.String() and l.blockchainID) in a short
retry/backoff loop: on error use l.logger.Error(...) to record the failure
(including error, blockchainID and contractAddress), sleep with exponential or
fixed backoff (and respect l.ctx cancellation if present) and retry until
success or context cancellation, only returning an error if the context is
canceled—do not return immediately on the first getLatestEvent error.
- Around line 151-153: The ctx.Done() branch in the event listener returns
without unsubscribing the subscription, leaking the subscription created as
eventSubscription; before returning from the ctx.Done() case, call
eventSubscription.Unsubscribe() (handle and log any unsubscribe error via
l.logger) similar to the error branch, ensuring the subscription is always
cleaned up on shutdown.
In `@pkg/rpc/types.go`:
- Around line 286-287: BlockchainInfoV1 currently replaces the previously
exposed validator address with LockingContractAddress which breaks the V1 wire
contract; restore the original validator address field in the BlockchainInfoV1
struct (keep the new LockingContractAddress as an additional field) and update
the V1 serialization logic (the node_v1 utils serializer that populates
BlockchainInfoV1) to emit both the original validator address field and the new
LockingContractAddress so existing V1 clients continue to receive the old field
while new clients can read the locking address.
In `@sdk/ts/src/client.ts`:
- Around line 1756-1764: The initializeLockingClient() flow is forcing a
Node-only TransactionSigner.getAccount() even for read-only operations like
getLockedBalance(); update initializeLockingClient() and related call sites so
the public client is created unconditionally but the walletClient
(createWalletClient(..., account)) is only created when txSigner.getAccount()
exists (or when a write operation is invoked), and make the signer/wallet client
optional in the locking client constructor (e.g., locking client factory used by
getLockedBalance() should accept undefined wallet signer). Specifically, in
initializeLockingClient(), split public-client creation from wallet-client
creation, guard createWalletClient() behind a getAccount() check, and ensure
getLockedBalance() (and other read paths) use the public client only and handle
an undefined wallet client gracefully.
---
Outside diff comments:
In `@pkg/blockchain/evm/channel_hub_reactor.go`:
- Around line 103-139: The switch currently calls stub handlers like
handleMigrationInFinalized, handleMigrationOutInitiated,
handleMigrationOutFinalized, handleDeposited, handleWithdrawn, and
handleEscrowDepositsPurged which only log and return nil, causing HandleEvent()
to persist them as processed; change this so TODO-only handlers do not mark
events as successfully handled: either implement the real handler logic, or have
each stub return a sentinel error (e.g. ErrUnimplemented) and update the calling
code around the switch (the block that calls r.onEventProcessed(...) and
r.storeContractEvent(...)) to treat ErrUnimplemented as a non-success that skips
onEventProcessed/storeContractEvent (and log accordingly). Reference the
functions handleMigrationInFinalized, handleMigrationOutInitiated,
handleMigrationOutFinalized, handleDeposited, handleWithdrawn,
handleEscrowDepositsPurged and the post-switch persistence logic to locate where
to change behavior.
In `@sdk/ts/src/core/types.ts`:
- Around line 164-169: Update the Blockchain interface so channelHubAddress is
optional (change channelHubAddress: Address to channelHubAddress?: Address) to
match backend semantics, and adjust the transformer that maps
info.channel_hub_address (the cast in utils.ts currently doing
info.channel_hub_address as Address) to defensively handle empty
string/undefined (e.g., check truthiness and set undefined or a valid Address
accordingly) so code consuming Blockchain (and the existing
lockingContractAddress checks) type-checks correctly without unsafe casts.
---
Duplicate comments:
In `@clearnode/main.go`:
- Around line 31-33: rootCtx is created as a non-cancelable context which causes
a race where cancelBlockchain() signals workers but runStoreMetricsExporter and
other goroutines still use context.Background() and may access bb.DbStore while
bb.Close() runs; change rootCtx to a cancelable context (use
context.WithCancel), propagate that cancelable ctx (the returned ctx variable)
into runStoreMetricsExporter and all background goroutines instead of
context.Background(), call the cancel function before bb.Close(), and ensure you
wait for those goroutines to exit (e.g., sync.WaitGroup or channel coordination
used by the listeners/workers referenced in blockchainCtx/cancelBlockchain and
runStoreMetricsExporter) before calling bb.Close() to avoid accesses to
bb.DbStore during teardown; apply the same fix where rootCtx/ctx/blockchainCtx
are created and used (rootCtx, blockchainCtx, cancelBlockchain, ctx,
runStoreMetricsExporter, bb.Close, bb.DbStore).
In `@clearnode/store/memory/blockchain_config.go`:
- Around line 86-88: The new validation allows blockchains without
ChannelHubAddress but runtime.go still assumes every blockchain has a ChannelHub
and crashes; revert to the stricter check in verifyBlockchainsConfig so configs
must include channel_hub_address (i.e., require bc.ChannelHubAddress != ""), or
alternatively update the startup logic in runtime.go's ChannelHub verification
loop (the code iterating GetBlockchains()) to skip ChannelHub checks for entries
where bc.ChannelHubAddress is empty and only validate locking_contract_address
where provided; reference verifyBlockchainsConfig, GetBlockchains,
ChannelHubAddress, LockingContractAddress and the ChannelHub verification block
in runtime.go when making the change.
In `@pkg/blockchain/evm/blockchain_client.go`:
- Around line 234-240: The response is incorrectly setting
EscrowDepositDataResponse.Node to the ChannelHub contract address
(c.channelHubContractAddress.Hex()); change it to return the
counterparty/participant node address instead (the same value
GetHomeChannelData() expects in Node). Locate where EscrowDepositDataResponse is
constructed (the block that returns EscrowDepositDataResponse with
EscrowChannelID, Node, LastState, UnlockExpiry, ChallengeExpiry) and replace
Node: c.channelHubContractAddress.Hex() with the counterparty address from the
channel/lastState/escrow data (e.g., use the participant/counterparty field
available on lastState or the escrow data), and make the same fix in the
analogous return at lines 259-263. Ensure the Node field contains the peer
node's address string, not the contract address.
---
Nitpick comments:
In `@cerebro/commands.go`:
- Around line 1290-1307: Validate the destination address in
withdrawSecurityTokens before calling o.client.WithdrawSecurityTokens: use the
same address validation utility the project uses (e.g., common.IsHexAddress or
the repo's address validation helper) to reject malformed or empty inputs and
print a clear error via fmt.Printf; keep parseChainID and the subsequent flow
intact but return early on invalid destination so the SDK isn't called with a
possibly zero/incorrect address.
- Around line 1334-1350: In securityBalance, validate the user-supplied wallet
string before calling o.client.GetLockedBalance: ensure the wallet is non-empty
and is a valid hex Ethereum address (e.g., use common.IsHexAddress or equivalent
validation) and if invalid print an error and return; reference the
securityBalance function and the caller that passes args[3] so the fix prevents
malformed input reaching LockingClient.GetLockedBalance which uses
common.HexToAddress silently.
In `@cerebro/operator.go`:
- Line 206: The entry {Text: "exit", Description: "Exit the CLI"} has
inconsistent indentation; align it with the surrounding slice/struct entries by
adding the missing leading tab(s) so it matches the indentation style used by
nearby entries (e.g., other command items in the same slice or block) inside
operator.go—locate the block containing the command definitions (the literal
with Text/Description pairs) and adjust the indentation of the "exit" line to be
consistent.
In `@clearnode/store/memory/blockchain_config_test.go`:
- Around line 18-51: Add unit tests in blockchain_config_test.go to cover the
two negative branches of verifyBlockchainsConfig: (1) a BlockchainConfig that
has neither ChannelHubAddress nor LockingContractAddress and should return an
error, and (2) a BlockchainConfig with a malformed LockingContractAddress
(invalid hex/length) which should also return an error; construct
BlockchainsConfig test cases that set ID/Name and the relevant address fields,
call the same validation path (verifyBlockchainsConfig or via the existing
loader used in tests), assert that an error is returned and that the error
message discriminates the case, and ensure other expectations like
defaultBlockStep remain unchanged for non-error cases (use BlockchainsConfig,
BlockchainConfig, verifyBlockchainsConfig, defaultBlockStep to locate code).
In `@pkg/blockchain/evm/listener_test.go`:
- Around line 67-69: Add a regression test that verifies Listen forwards non-nil
callback errors to handleClosure: define a sentinel error (e.g., var sentinel =
errors.New("sentinel")), set handleEvent to return that sentinel instead of
closing eventHandled, stub or capture handleClosure to receive errors, invoke
Listen (using the same setup as the existing test), wait for handleClosure to be
called, and assert the received error equals the sentinel; update the test case
in listener_test.go to include this new branch so handleEvent -> Listen ->
handleClosure error propagation is exercised.
In `@pkg/core/interface.go`:
- Around line 50-61: The interface name AppRegistryClient in this file should be
renamed to LockingClient to match the rest of the PR and avoid a breaking public
API rename later; update the type declaration `type AppRegistryClient interface
{ ... }` to `type LockingClient interface { ... }` and ensure any references in
the codebase (including usages alongside LockingContractReactor and
LockingContractAddress) are updated to the new symbol so the public core API
consistently uses the "Locking" terminology.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa503f35-8bb9-421a-b701-7d907b94f3a2
📒 Files selected for processing (48)
cerebro/commands.gocerebro/main.gocerebro/operator.gocerebro/operator_test.gocerebro/storage.goclearnode/api/node_v1/utils.goclearnode/blockchain_worker.goclearnode/chart/config/v1-rc/assets.yamlclearnode/chart/config/v1-rc/blockchains.yamlclearnode/event_handlers/interface.goclearnode/event_handlers/service.goclearnode/event_handlers/service_test.goclearnode/event_handlers/testing.goclearnode/main.goclearnode/store/database/database.goclearnode/store/memory/blockchain_config.goclearnode/store/memory/blockchain_config_test.goclearnode/store/memory/memory_store.gopkg/blockchain/evm/app_registry_abi.gopkg/blockchain/evm/base_client.gopkg/blockchain/evm/blockchain_client.gopkg/blockchain/evm/blockchain_test.gopkg/blockchain/evm/channel_hub_abi.gopkg/blockchain/evm/channel_hub_reactor.gopkg/blockchain/evm/client_opts.gopkg/blockchain/evm/erc20_abi.gopkg/blockchain/evm/init.gopkg/blockchain/evm/interface.gopkg/blockchain/evm/listener.gopkg/blockchain/evm/listener_test.gopkg/blockchain/evm/locking_client.gopkg/blockchain/evm/locking_reactor.gopkg/blockchain/evm/locking_reactor_test.gopkg/core/event.gopkg/core/interface.gopkg/core/types.gopkg/rpc/types.gosdk/go/channel.gosdk/go/client.gosdk/go/utils.gosdk/ts/src/blockchain/evm/app_registry_abi.tssdk/ts/src/blockchain/evm/index.tssdk/ts/src/blockchain/evm/locking_client.tssdk/ts/src/client.tssdk/ts/src/core/types.tssdk/ts/src/rpc/types.tssdk/ts/src/signers.tssdk/ts/src/utils.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- clearnode/event_handlers/interface.go
- clearnode/event_handlers/service.go
- clearnode/event_handlers/testing.go
- sdk/ts/src/blockchain/evm/app_registry_abi.ts
- sdk/ts/src/utils.ts
- sdk/go/utils.go
- pkg/blockchain/evm/blockchain_test.go
- pkg/core/types.go
- clearnode/api/node_v1/utils.go
- clearnode/store/database/database.go
- cerebro/operator_test.go
- sdk/ts/src/rpc/types.ts
- clearnode/event_handlers/service_test.go
- pkg/blockchain/evm/locking_reactor_test.go
- pkg/blockchain/evm/locking_client.go
- pkg/blockchain/evm/base_client.go
- clearnode/chart/config/v1-rc/assets.yaml
dimast-x
force-pushed
the
feat/integrate-locking
branch
from
8f5897a to
024a858
Compare
Contributor
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/store/memory/memory_store.go (1)
129-135:⚠️ Potential issue | 🔴 CriticalPersist
channelSigValidatorsonMemoryStoreV1.
NewMemoryStoreV1collects validators intochannelSigValidators, but the returned store never assigns that field.GetChannelSigValidatorswill therefore miss every blockchain, andmain.gowill fatal on startup when it tries to register validators.Minimal fix
return &MemoryStoreV1{ - blockchains: blockchains, - assets: assets, - supportedAssets: supportedAssets, - tokenDecimals: tokenDecimals, - assetDecimals: assetDecimals, + blockchains: blockchains, + assets: assets, + channelSigValidators: channelSigValidators, + supportedAssets: supportedAssets, + tokenDecimals: tokenDecimals, + assetDecimals: assetDecimals, }, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/memory/memory_store.go` around lines 129 - 135, NewMemoryStoreV1 builds channelSigValidators but never sets it on the returned MemoryStoreV1, so GetChannelSigValidators returns empty and main.go fails when registering validators; update the MemoryStoreV1 literal returned by NewMemoryStoreV1 to include channelSigValidators: channelSigValidators: channelSigValidators (or whatever local variable name is used), ensuring the field on the struct is populated so GetChannelSigValidators can return the collected validators.
♻️ Duplicate comments (3)
clearnode/main.go (1)
198-202:⚠️ Potential issue | 🟠 MajorWait for blockchain goroutines to exit before
bb.Close().
cancelBlockchain()only signals the listeners/workers.bb.Close()still runs immediately afterward, so they can still be insideStoreContractEvent,GetLatestEvent, or RPC calls during teardown. Add aWaitGroupor done-channel join before closing the backbone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/main.go` around lines 198 - 202, The shutdown currently calls cancelBlockchain() to signal blockchain listeners/workers but immediately calls bb.Close(), which can race with in-flight calls like StoreContractEvent or GetLatestEvent; modify the shutdown flow to wait for all blockchain goroutines to finish before calling bb.Close(): add a sync.WaitGroup (or a done channel) that each blockchain listener/worker increments/decrements (or signals) and have cancelBlockchain() still send the cancellation, then block on wg.Wait() (or receive from the done channel) in the same shutdown path before invoking bb.Close(); update the listeners/workers that spawn goroutines (the functions that call StoreContractEvent/GetLatestEvent) to use the WaitGroup/done-channel so they reliably signal completion.clearnode/store/memory/memory_store.go (1)
29-32:⚠️ Potential issue | 🟠 MajorDon't exclude lock-only chains from the supported blockchain set.
verifyBlockchainsConfignow accepts chains that only definelocking_contract_address, but this guard still filters them out ofsupportedAssets,tokenDecimals, andSuggestedBlockchainIDresolution. Tokens configured only on a lock-only chain become unreachable.Suggested fix
- if bc.ChannelHubAddress != "" { - supportedBlockchainIDs[bc.ID] = struct{}{} - channelSigValidators[bc.ID] = bc.ChannelHubSigValidators - } + if bc.ChannelHubAddress != "" || bc.LockingContractAddress != "" { + supportedBlockchainIDs[bc.ID] = struct{}{} + } + if bc.ChannelHubAddress != "" { + channelSigValidators[bc.ID] = bc.ChannelHubSigValidators + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/memory/memory_store.go` around lines 29 - 32, The current guard that adds a blockchain to supportedBlockchainIDs and channelSigValidators only when bc.ChannelHubAddress != "" excludes lock-only chains; instead, always register the chain ID in supportedBlockchainIDs (i.e., set supportedBlockchainIDs[bc.ID] = struct{}{} for every bc), and only assign channelSigValidators[bc.ID] = bc.ChannelHubSigValidators when the validator data is present (e.g., if bc.ChannelHubSigValidators != nil or bc.ChannelHubAddress != ""), so that verifyBlockchainsConfig-accepted chains that are lock-only are reachable by supportedAssets, tokenDecimals and SuggestedBlockchainID resolution.pkg/blockchain/evm/blockchain_client.go (1)
240-240:⚠️ Potential issue | 🟠 MajorReturn the participant node in escrow responses, not the ChannelHub contract.
The home-channel response in this file still uses the channel node address, but these two escrow responses now fill
Nodewithc.channelHubContractAddress.Hex(). Callers that treatNodeas the counterparty will read the contract address instead.Minimal fix
- Node: c.channelHubContractAddress.Hex(), + Node: c.nodeAddress.Hex(),Apply the same change in both response builders.
Also applies to: 265-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/blockchain/evm/blockchain_client.go` at line 240, The escrow response builders currently set the Node field to c.channelHubContractAddress.Hex(), which returns the contract address instead of the participant/counterparty; change the Node assignment in both response builders to use the participant/counterparty address (the participant address variable used in those builders, e.g. participant.Hex() or counterparty.Hex()) rather than c.channelHubContractAddress.Hex() so callers receive the actual participant node in the Node field.
🧹 Nitpick comments (1)
clearnode/store/memory/blockchain_config_test.go (1)
10-122: Add regression cases for the new config rules.This table still does not cover the new failure paths for: missing both contract addresses, invalid
locking_contract_address, and missing/invalidchannel_hub_sig_validators. Those are the branches most likely to regress in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/memory/blockchain_config_test.go` around lines 10 - 122, Add regression test cases to TestBlockchainConfig_verifyVariables covering the new failure paths: (1) a blockchain config missing both ChannelHubAddress and LockingContractAddress should produce an error (use BlockchainsConfig/BlockchainConfig with neither ChannelHubAddress nor LockingContractAddress set and assert expectedErrorStr accordingly), (2) a config with an invalid LockingContractAddress (malformed hex) should produce the specific invalid locking contract address error, and (3) configs with missing or invalid ChannelHubSigValidators for chains that require them (empty or malformed ChannelHubSigValidators map) should produce the corresponding validation error; add these cases to the tcs slice with appropriate expectedErrorStr values and optional assertFunc where successful validation is expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clearnode/store/memory/interface.go`:
- Around line 17-18: The MemoryStore interface gained a new method
GetChannelSigValidators(blockchainID uint64) (map[uint8]string, error) which
breaks the test doubles; update the MockMemoryStore types in
clearnode/api/node_v1/testing.go and clearnode/api/channel_v1/testing.go to
implement this method. Add a simple stub on each MockMemoryStore named
GetChannelSigValidators that accepts blockchainID uint64 and returns an empty
map (or a configurable map) and nil error so the mocks satisfy
memory.MemoryStore; ensure the method signature exactly matches the interface.
In `@pkg/blockchain/evm/blockchain_client.go`:
- Around line 746-781: The code in EnsureSigValidatorRegistered currently signs
the packed registration message with c.txSigner (c.txSigner.Sign), which is the
gas/transaction signer, causing mismatched-authority signatures when
c.nodeAddress is backed by the state/node signer; replace the use of
c.txSigner.Sign with the signer tied to c.nodeAddress (the node/state signer
field on BlockchainClient, e.g., c.nodeSigner or c.stateSigner) so the signature
is produced by the key that actually controls c.nodeAddress, and ensure you call
that signer's Sign method to produce sig before calling
c.contract.RegisterNodeValidator.
---
Outside diff comments:
In `@clearnode/store/memory/memory_store.go`:
- Around line 129-135: NewMemoryStoreV1 builds channelSigValidators but never
sets it on the returned MemoryStoreV1, so GetChannelSigValidators returns empty
and main.go fails when registering validators; update the MemoryStoreV1 literal
returned by NewMemoryStoreV1 to include channelSigValidators:
channelSigValidators: channelSigValidators (or whatever local variable name is
used), ensuring the field on the struct is populated so GetChannelSigValidators
can return the collected validators.
---
Duplicate comments:
In `@clearnode/main.go`:
- Around line 198-202: The shutdown currently calls cancelBlockchain() to signal
blockchain listeners/workers but immediately calls bb.Close(), which can race
with in-flight calls like StoreContractEvent or GetLatestEvent; modify the
shutdown flow to wait for all blockchain goroutines to finish before calling
bb.Close(): add a sync.WaitGroup (or a done channel) that each blockchain
listener/worker increments/decrements (or signals) and have cancelBlockchain()
still send the cancellation, then block on wg.Wait() (or receive from the done
channel) in the same shutdown path before invoking bb.Close(); update the
listeners/workers that spawn goroutines (the functions that call
StoreContractEvent/GetLatestEvent) to use the WaitGroup/done-channel so they
reliably signal completion.
In `@clearnode/store/memory/memory_store.go`:
- Around line 29-32: The current guard that adds a blockchain to
supportedBlockchainIDs and channelSigValidators only when bc.ChannelHubAddress
!= "" excludes lock-only chains; instead, always register the chain ID in
supportedBlockchainIDs (i.e., set supportedBlockchainIDs[bc.ID] = struct{}{} for
every bc), and only assign channelSigValidators[bc.ID] =
bc.ChannelHubSigValidators when the validator data is present (e.g., if
bc.ChannelHubSigValidators != nil or bc.ChannelHubAddress != ""), so that
verifyBlockchainsConfig-accepted chains that are lock-only are reachable by
supportedAssets, tokenDecimals and SuggestedBlockchainID resolution.
In `@pkg/blockchain/evm/blockchain_client.go`:
- Line 240: The escrow response builders currently set the Node field to
c.channelHubContractAddress.Hex(), which returns the contract address instead of
the participant/counterparty; change the Node assignment in both response
builders to use the participant/counterparty address (the participant address
variable used in those builders, e.g. participant.Hex() or counterparty.Hex())
rather than c.channelHubContractAddress.Hex() so callers receive the actual
participant node in the Node field.
---
Nitpick comments:
In `@clearnode/store/memory/blockchain_config_test.go`:
- Around line 10-122: Add regression test cases to
TestBlockchainConfig_verifyVariables covering the new failure paths: (1) a
blockchain config missing both ChannelHubAddress and LockingContractAddress
should produce an error (use BlockchainsConfig/BlockchainConfig with neither
ChannelHubAddress nor LockingContractAddress set and assert expectedErrorStr
accordingly), (2) a config with an invalid LockingContractAddress (malformed
hex) should produce the specific invalid locking contract address error, and (3)
configs with missing or invalid ChannelHubSigValidators for chains that require
them (empty or malformed ChannelHubSigValidators map) should produce the
corresponding validation error; add these cases to the tcs slice with
appropriate expectedErrorStr values and optional assertFunc where successful
validation is expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89b05563-e21c-46cc-bdf9-3190e2a5fc7b
📒 Files selected for processing (8)
clearnode/chart/config/v1-rc/blockchains.yamlclearnode/main.goclearnode/store/memory/blockchain_config.goclearnode/store/memory/blockchain_config_test.goclearnode/store/memory/interface.goclearnode/store/memory/memory_store.gopkg/blockchain/evm/blockchain_client.gopkg/core/interface.go
dimast-x
force-pushed
the
feat/integrate-locking
branch
3 times, most recently
from
378360a to
286b71c
Compare
dimast-x
changed the title
dimast-x
force-pushed
the
feat/integrate-locking
branch
from
286b71c to
eb6db9e
Compare
philanton
approved these changes
Mar 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactors