Use permanent node index from NodeRegistry contract#4783
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…d-from-node-registry-contract # Conflicts: # packages/contracts/src/river/registry/facets/node/NodeRegistry.sol # packages/contracts/test/river/registry/node/NodeRegistry.t.sol
📝 WalkthroughWalkthroughReplaces in-memory auto-increment permanentIndex with contract-supplied permanentIndex across node registry: addNodeLocked accepts a permanentIndex, LoadNodeRegistry and OnNodeAdded propagate node.PermanentIndex from the contract, and nextPermanentIndexLocked is removed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/node/nodes/node_registry.go (1)
49-53: Dead field:nextPermanentIndexLockedis no longer used.The field at line 53 is still declared but never initialized or referenced after this refactor. Remove it to avoid confusion.
Proposed fix
mu sync.RWMutex nodesLocked map[common.Address]*NodeRecord nodesByIndexLocked map[int32]*NodeRecord appliedBlockNumLocked blockchain.BlockNumber - nextPermanentIndexLocked int
🧹 Nitpick comments (1)
core/node/nodes/node_registry.go (1)
195-206: Consider specifying block number for contract call consistency.The
GetNodecall reads from the latest block, while the event was emitted ate.Raw.BlockNumber. For immutable fields likePermanentIndex, this is likely fine, but for consistency, consider usingBlockNumber: big.NewInt(int64(e.Raw.BlockNumber))inCallOpts.Also, if this call fails, the node isn't added until a restart. Ensure this is acceptable behavior.
Optional: Specify block number
- node, err := n.contract.NodeRegistry.GetNode(&bind.CallOpts{Context: ctx}, e.NodeAddress) + node, err := n.contract.NodeRegistry.GetNode(&bind.CallOpts{ + Context: ctx, + BlockNumber: new(big.Int).SetUint64(e.Raw.BlockNumber), + }, e.NodeAddress)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/node/nodes/node_registry.go
🧰 Additional context used
📓 Path-based instructions (3)
core/**/*.go
📄 CodeRabbit inference engine (core/AGENTS.md)
core/**/*.go: Format all modified Go files using ./fmt.sh from the /core directory before committing
Run the linter using ./lint.sh from the /core directory to check code quality, which runs both golangci-lint and staticcheck
Use RiverError type from node/base/error.go for structured errors
Use RiverErrorWithBase to construct a river error derived from an error of arbitrary or unknown type
Wrap blockchain errors with context using crypto/ utilities
Log errors with structured fields using zap logger
Use node/crypto/blockchain.go for Ethereum client operations
Use node/crypto/chain_monitor.go for transaction monitoring
Use xchain/entitlement/ for cross-chain entitlement checks
Do not run ./fmt.sh and ./lint.sh at the same time; run them as separate commands
Use go.uber.org/zap for structured logging with custom extensions from node/logging/
Use go.opentelemetry.io for distributed tracing across the application
Use prometheus for OpenMetrics in application metrics collection
Files:
core/node/nodes/node_registry.go
**/core/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/core/**/*.go: Format all Go files using the fmt.sh script from the /core directory
Run the linter using the lint.sh script from the /core directory for all Go code changes
Use structured logging with zap for Go development
Implement proper error handling with RiverError types in Go code
Implement proper context cancellation for graceful shutdowns in Go code
Files:
core/node/nodes/node_registry.go
**/core/node/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use structured logging with zap in Go backend River nodes for observability
Files:
core/node/nodes/node_registry.go
🧠 Learnings (8)
📚 Learning: 2025-12-17T02:40:57.825Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/AGENTS.md:0-0
Timestamp: 2025-12-17T02:40:57.825Z
Learning: Applies to core/**/*.go : Use node/crypto/blockchain.go for Ethereum client operations
Applied to files:
core/node/nodes/node_registry.go
📚 Learning: 2025-12-17T02:40:57.825Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/AGENTS.md:0-0
Timestamp: 2025-12-17T02:40:57.825Z
Learning: Applies to core/node/crypto/**/*.go : Use github.com/ethereum/go-ethereum for blockchain integration
Applied to files:
core/node/nodes/node_registry.go
📚 Learning: 2025-12-17T02:40:57.825Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/AGENTS.md:0-0
Timestamp: 2025-12-17T02:40:57.825Z
Learning: Applies to core/node/rpc/**/*.go : Use connectrpc.com/connect (Connect protocol) for HTTP/gRPC compatibility in service implementations
Applied to files:
core/node/nodes/node_registry.go
📚 Learning: 2025-12-17T02:40:57.825Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/AGENTS.md:0-0
Timestamp: 2025-12-17T02:40:57.825Z
Learning: Applies to core/**/*.go : Wrap blockchain errors with context using crypto/ utilities
Applied to files:
core/node/nodes/node_registry.go
📚 Learning: 2025-12-17T02:40:57.825Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/AGENTS.md:0-0
Timestamp: 2025-12-17T02:40:57.825Z
Learning: Applies to core/**/*.go : Use xchain/entitlement/ for cross-chain entitlement checks
Applied to files:
core/node/nodes/node_registry.go
📚 Learning: 2025-11-25T08:47:37.754Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/node/rpc/AGENTS.md:0-0
Timestamp: 2025-11-25T08:47:37.754Z
Learning: Applies to core/node/rpc/**/*_test.go : Create authenticated clients using protocolconnect service clients with the service address and HTTP client, and use `authenticateBS()` to add authentication to requests
Applied to files:
core/node/nodes/node_registry.go
📚 Learning: 2025-12-17T02:40:57.825Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/AGENTS.md:0-0
Timestamp: 2025-12-17T02:40:57.825Z
Learning: Applies to core/**/*.go : Use node/crypto/chain_monitor.go for transaction monitoring
Applied to files:
core/node/nodes/node_registry.go
📚 Learning: 2025-11-25T08:48:08.084Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/node/storage/AGENTS.md:0-0
Timestamp: 2025-11-25T08:48:08.084Z
Learning: Applies to core/node/storage/storage/**/*_test.go : Use standard imports in storage tests: context, testing, github.com/ethereum/go-ethereum/common, github.com/stretchr/testify/require, and relevant River protocol packages
Applied to files:
core/node/nodes/node_registry.go
🧬 Code graph analysis (1)
core/node/nodes/node_registry.go (2)
core/node/registries/river_registry_contract.go (1)
NodeRecord(729-729)core/node/nodes/node_record.go (1)
NodeRecord(13-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build_Anvil_Docker
- GitHub Check: XChain_Integration
🔇 Additional comments (3)
core/node/nodes/node_registry.go (3)
88-97: LGTM!Clean initialization that properly aligns with the new permanentIndex-from-contract approach.
163-189: LGTM!Signature properly updated to accept contract-provided
permanentIndex. Theuint32tointconversion is safe.
357-364: LGTM!Clone initialization correctly updated to match the new struct layout.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/node/nodes/node_registry.go (2)
167-178: Type conversion note.
uint32tointis safe on 64-bit systems. On 32-bit systems, values > 2^31-1 would overflow. Likely not an issue in practice, but consider usingint32orint64forpermanentIndexinNodeRecordto be explicit.
194-200: Blocking contract call in event handler.This adds latency to event processing. The TODO indicates it's temporary, but consider adding a timeout via
context.WithTimeoutif this persists, to avoid blocking indefinitely on RPC issues.💡 Suggested timeout wrapper
+ callCtx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + - node, err := n.contract.NodeRegistry.GetNode(&bind.CallOpts{Context: ctx}, e.NodeAddress) + node, err := n.contract.NodeRegistry.GetNode(&bind.CallOpts{Context: callCtx}, e.NodeAddress)Don't forget to add
"time"to imports if applied.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/node/nodes/node_registry.go
🧰 Additional context used
📓 Path-based instructions (3)
core/**/*.go
📄 CodeRabbit inference engine (core/AGENTS.md)
core/**/*.go: Format all modified Go files using ./fmt.sh from the /core directory before committing
Run the linter using ./lint.sh from the /core directory to check code quality, which runs both golangci-lint and staticcheck
Use RiverError type from node/base/error.go for structured errors
Use RiverErrorWithBase to construct a river error derived from an error of arbitrary or unknown type
Wrap blockchain errors with context using crypto/ utilities
Log errors with structured fields using zap logger
Use node/crypto/blockchain.go for Ethereum client operations
Use node/crypto/chain_monitor.go for transaction monitoring
Use xchain/entitlement/ for cross-chain entitlement checks
Do not run ./fmt.sh and ./lint.sh at the same time; run them as separate commands
Use go.uber.org/zap for structured logging with custom extensions from node/logging/
Use go.opentelemetry.io for distributed tracing across the application
Use prometheus for OpenMetrics in application metrics collection
Files:
core/node/nodes/node_registry.go
**/core/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/core/**/*.go: Format all Go files using the fmt.sh script from the /core directory
Run the linter using the lint.sh script from the /core directory for all Go code changes
Use structured logging with zap for Go development
Implement proper error handling with RiverError types in Go code
Implement proper context cancellation for graceful shutdowns in Go code
Files:
core/node/nodes/node_registry.go
**/core/node/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use structured logging with zap in Go backend River nodes for observability
Files:
core/node/nodes/node_registry.go
🧠 Learnings (8)
📚 Learning: 2025-12-17T02:40:57.825Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/AGENTS.md:0-0
Timestamp: 2025-12-17T02:40:57.825Z
Learning: Applies to core/**/*.go : Use node/crypto/blockchain.go for Ethereum client operations
Applied to files:
core/node/nodes/node_registry.go
📚 Learning: 2025-12-17T02:40:57.825Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/AGENTS.md:0-0
Timestamp: 2025-12-17T02:40:57.825Z
Learning: Applies to core/node/crypto/**/*.go : Use github.com/ethereum/go-ethereum for blockchain integration
Applied to files:
core/node/nodes/node_registry.go
📚 Learning: 2025-12-17T02:40:57.825Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/AGENTS.md:0-0
Timestamp: 2025-12-17T02:40:57.825Z
Learning: Applies to core/node/rpc/**/*.go : Use connectrpc.com/connect (Connect protocol) for HTTP/gRPC compatibility in service implementations
Applied to files:
core/node/nodes/node_registry.go
📚 Learning: 2025-12-17T02:40:57.825Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/AGENTS.md:0-0
Timestamp: 2025-12-17T02:40:57.825Z
Learning: Applies to core/**/*.go : Wrap blockchain errors with context using crypto/ utilities
Applied to files:
core/node/nodes/node_registry.go
📚 Learning: 2025-12-17T02:40:57.825Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/AGENTS.md:0-0
Timestamp: 2025-12-17T02:40:57.825Z
Learning: Applies to core/**/*.go : Use xchain/entitlement/ for cross-chain entitlement checks
Applied to files:
core/node/nodes/node_registry.go
📚 Learning: 2025-11-25T08:47:37.754Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/node/rpc/AGENTS.md:0-0
Timestamp: 2025-11-25T08:47:37.754Z
Learning: Applies to core/node/rpc/**/*_test.go : Create authenticated clients using protocolconnect service clients with the service address and HTTP client, and use `authenticateBS()` to add authentication to requests
Applied to files:
core/node/nodes/node_registry.go
📚 Learning: 2025-12-17T02:40:57.825Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/AGENTS.md:0-0
Timestamp: 2025-12-17T02:40:57.825Z
Learning: Applies to core/**/*.go : Use node/crypto/chain_monitor.go for transaction monitoring
Applied to files:
core/node/nodes/node_registry.go
📚 Learning: 2025-11-25T08:48:08.084Z
Learnt from: CR
Repo: towns-protocol/towns PR: 0
File: core/node/storage/AGENTS.md:0-0
Timestamp: 2025-11-25T08:48:08.084Z
Learning: Applies to core/node/storage/storage/**/*_test.go : Use standard imports in storage tests: context, testing, github.com/ethereum/go-ethereum/common, github.com/stretchr/testify/require, and relevant River protocol packages
Applied to files:
core/node/nodes/node_registry.go
🧬 Code graph analysis (1)
core/node/nodes/node_registry.go (3)
core/node/registries/river_registry_contract.go (1)
NodeRecord(729-729)core/node/nodes/node_record.go (1)
NodeRecord(13-22)core/blockchain/block_number.go (1)
BlockNumber(5-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Multinode_Ent_Legacy
- GitHub Check: Multinode_Ent
- GitHub Check: Multinode
- GitHub Check: Go_Tests
- GitHub Check: Common_CI
- GitHub Check: XChain_Integration
🔇 Additional comments (5)
core/node/nodes/node_registry.go (5)
11-11: LGTM!Import correctly added for
bind.CallOptsusage inOnNodeAdded.
49-52: LGTM!Clean removal of
nextPermanentIndexLocked. Registry now correctly relies on contract-provided indices.
87-100: LGTM!Initialization correctly simplified. Contract-provided
PermanentIndexnow flows through properly.
356-391: LGTM!Clone correctly rebuilds
nodesByIndexLockedfrom node data rather than relying on a counter.
150-152: Add code comment explainingpermanentIndex > 0check.The condition at lines 150 and 388 excludes nodes with
permanentIndex == 0from the index lookup map. This appears intentional (likely treating 0 as "unset"), but lacks documentation. If permanentIndex can legitimately be 0, those nodes become unretrievable viaGetNodeByPermanentIndex. Add a comment clarifying whether 0 is a reserved/unset value or if this is a bug.
1 participant
No description provided.