Breaking!: Integrate NodesV2 #589
Conversation
WalkthroughThis pull request modifies CLI command-line argument parsing, updates the structure of options, and enhances node management functionalities. Key changes include renaming command-line flags, restructuring the Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Command Line Interface
participant NodeManager as Node Management Module
participant NodeRegistry as Node Registry
participant Logger as Logging System
CLI->>NodeManager: Parse command arguments
NodeManager->>NodeRegistry: Validate node options
alt Node options valid
NodeRegistry-->>NodeManager: Return node details
NodeManager->>Logger: Log success message
else Node options invalid
NodeManager-->>CLI: Return error message
end
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24) ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
fbac
changed the title
fbac
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
pkg/testutils/contractRegistry.go (1)
1-23: Clarify “unhealthy” vs. “healthy” definitions.
Currently, “unhealthy” simply setsIsDisabled = truebut keeps other boolean fields true. This may be sufficient for your test coverage. However, consider setting additional fields (likeIsApiEnabled = false) to reflect multiple unhealthy states in your tests. This can improve coverage of different node conditions.pkg/registry/contractRegistry.go (1)
219-219: Validate new contract method name.
The rename fromAllNodestoGetAllNodesis consistent with the code changes. Ensure any mocks or stubs for testing also reflect this new method name.pkg/server/server_test.go (1)
102-102: Consider validating node functionality with disabled features.The tests currently only validate nodes with all features enabled. It would be beneficial to add test cases that verify the behavior when nodes have certain features disabled, such as API or replication, to ensure the system correctly handles these cases according to the PR objectives.
Add test cases that verify server behavior with nodes that have various feature flags disabled:
+func TestServerWithDisabledFeatures(t *testing.T) { + // Setup similar to TestCreateServer + // ... + + // Test with API disabled + apiDisabledNode := r.Node{ + NodeID: server1NodeID, + SigningKey: &privateKey1.PublicKey, + HttpAddress: fmt.Sprintf("http://localhost:%d", server1Port), + IsReplicationEnabled: true, + IsApiEnabled: false, // API disabled + IsDisabled: false, + MinMonthlyFee: big.NewInt(0), + IsValidConfig: true, + } + + // Test behavior with API disabled + // ... + + // Test with replication disabled + // ... + + // Test with completely disabled node + // ... +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cmd/cli/cli_test.go(1 hunks)cmd/cli/main.go(2 hunks)pkg/api/payer/nodeSelector.go(2 hunks)pkg/api/payer/nodeSelector_test.go(9 hunks)pkg/api/payer/publish_test.go(3 hunks)pkg/blockchain/interface.go(0 hunks)pkg/mocks/registry/mock_NodesContract.go(3 hunks)pkg/registry/contractRegistry.go(5 hunks)pkg/registry/contractRegistry_test.go(4 hunks)pkg/registry/interface.go(1 hunks)pkg/registry/node.go(5 hunks)pkg/server/server_test.go(3 hunks)pkg/sync/syncWorker.go(1 hunks)pkg/testutils/contractRegistry.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/blockchain/interface.go
✅ Files skipped from review due to trivial changes (2)
- cmd/cli/main.go
- cmd/cli/cli_test.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test (Node)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (27)
pkg/registry/interface.go (1)
5-5: Update to NodesV2 package for interface dependencyThis change aligns with the PR objective of integrating NodesV2 into the system.
pkg/api/payer/publish_test.go (3)
5-6: Added the missing testing importAdding the explicit import for the testing package is a good practice.
132-132: Enhanced test maintainability with standardized node creationUsing
testutils.GetHealthyNode(100)instead of direct instantiation of a node object improves maintainability and ensures consistent test data.
186-186: Enhanced test maintainability with standardized node creationSimilar to the previous change, using
testutils.GetHealthyNode(100)improves maintainability.pkg/registry/node.go (4)
6-6: Added math/big import for new MinMonthlyFee fieldThis import is required for the addition of the
*big.Inttype field.
50-50: Fixed error message capitalizationThe error message now follows proper capitalization conventions.
55-55: Fixed error message capitalizationThe error message now follows proper capitalization conventions.
74-74: Fixed error message capitalizationThe error message now follows proper capitalization conventions.
pkg/sync/syncWorker.go (1)
176-176:Details
✅ Verification successful
Updated node health check condition to align with NodesV2 integration
The condition has been changed from checking
!node.IsHealthy || !node.IsValidConfigto looking at more specific properties:node.IsDisabled || !node.IsReplicationEnabled || !node.IsValidConfig. This aligns with the PR objective to skip nodes that are disabled or don't have replication enabled.Let's verify that these new fields are properly initialized when nodes are created:
🏁 Script executed:
#!/bin/bash # Search for Node struct initialization patterns to verify proper field assignment rg "Node\{" --type go -A 10Length of output: 28747
Verified: Node health check condition and field assignments are correct
I reviewed the Node struct initializations across the codebase and confirmed that the new fields—
IsDisabled,IsReplicationEnabled, andIsValidConfig—are properly set. In particular:
- pkg/testutils/contractRegistry.go: Healthy nodes are initialized with
IsDisabled: false,IsReplicationEnabled: true, andIsValidConfig: true.- pkg/api/payer/nodeSelector_test.go: Similar patterns confirm that test nodes are created with the correct flag values.
The update in
pkg/sync/syncWorker.go(line 176) to check fornode.IsDisabled || !node.IsReplicationEnabled || !node.IsValidConfigis well aligned with these initializations and meets the PR objective to properly skip nodes that are disabled or lack replication enabled.Approved.
pkg/api/payer/nodeSelector.go (1)
7-8: Import looks good.
The addition of the"sort"import is appropriate and used bysort.Slice()below.pkg/registry/contractRegistry.go (3)
50-52: Good use of interface-implementation guard.
Declaringvar _ NodeRegistry = &SmartContractRegistry{}is a concise way to ensureSmartContractRegistryimplements theNodeRegistryinterface at compile time.
235-257: Expanded node fields are properly mapped.
The function correctly setsIsReplicationEnabled,IsApiEnabled,IsDisabled, andMinMonthlyFee. Verify that removed fields likeIsHealthyare no longer needed.
60-63:Details
❓ Verification inconclusive
NodesV2 transition is consistent.
Switching tonodesv2.NewNodesV2Calleraligns with the introduction of your new NodesV2 contracts. Ensure all references tonodescalls have been removed or updated throughout the codebase.Run this script to locate any leftover references to
nodes.:
🏁 Script executed:
#!/bin/bash rg --context 3 'nodes\.'Length of output: 51916
Ensure NodesV2 Transition Consistency and Clarify Legacy Uses
The update in
pkg/registry/contractRegistry.gocorrectly switches to usingnodesv2.NewNodesV2Caller, aligning with the new NodesV2 contracts. However, a recursive search fornodes\.reveals many remaining references (mainly in test utilities and legacy registry callers) that still invoke functions from the old nodes package. Please verify that these legacy calls are intentional—i.e., that support for NodesV1 is still required where applicable. If the goal is to fully migrate to NodesV2, then those instances will need further updates.
- The call in
pkg/registry/contractRegistry.gonow uses the new nodesv2 API as expected.- Multiple files (e.g., in
pkg/testutils,pkg/blockchain/registryCaller.go, and related tests) still referencenodes.functions.- Confirm whether the leftover legacy references are intended to coexist (supporting NodesV1) or if they should also be updated to nodesv2.
pkg/api/payer/nodeSelector_test.go (3)
62-76: Well-designed test case for an important edge condition.This test case verifies the behavior when all nodes are unhealthy, which is a critical edge case. It confirms that the system correctly returns the expected error message when no active nodes are available for assignment.
80-101: The node structure has been properly updated with new filtering fields.The test now includes the full set of node attributes necessary for testing the updated node selection logic, including
IsDisabled,IsReplicationEnabled,IsApiEnabled, andIsValidConfigfields. This allows for more comprehensive testing of the new node filtering criteria described in the PR objectives.
17-25: Helper utilities improve test code maintainability.In the test setup, there's a transition from hardcoded node structures to using utility functions
testutils.GetHealthyNode(). This improves maintainability by centralizing node creation logic.pkg/registry/contractRegistry_test.go (4)
6-14: Successfully migrated to NodesV2 with appropriate import changes.The file now correctly imports the
math/bigpackage to support the BigInt node IDs and has updated to use thenodesv2package instead of the previousnodespackage.
48-64: Successfully refactored to use the new NodesV2 API and data structures.The test has been updated to use the new
GetAllNodesmethod instead ofAllNodes, and it correctly handles the new return type ([]nodesv2.INodesNodeWithId). The node ID is now properly initialized usingbig.NewInt()instead of a uint32 value.
97-111: Contract registry tests properly handled method signature changes.The mock setup has been updated to use the proper return type and method name, maintaining test functionality while adapting to the breaking changes in the NodesV2 API.
146-154:Details
✅ Verification successful
Verify that all other callers of GetAllNodes are updated.
The breaking change from
AllNodestoGetAllNodesrequires updates across the codebase. Let's verify all call sites have been properly updated.
🏁 Script executed:
#!/bin/bash # Check for any remaining references to AllNodes that might need updating grep -r "AllNodes" --include="*.go" . | grep -v "mock_"Length of output: 30538
Evaluation of GetAllNodes update consistency across the codebase
The grep search confirms that all new functionality and tests now use the updated
GetAllNodesAPI. In particular, the updated test inpkg/registry/contractRegistry_test.goand the V2 methods in the nodesV2 bindings and registry caller are all usingGetAllNodes. The remaining references toAllNodesappear only in legacy V1 contexts or in auto‑generated contract bindings (e.g., incontracts/pkg/nodes/Nodes.goand the V1 methods inpkg/blockchain/registryCaller.go), which we assume are maintained for backward compatibility.No further changes are required here as long as the legacy V1 usage is intentional. Please confirm that supporting both V1 and V2 is by design.
pkg/server/server_test.go (3)
9-9: Added necessary import for big.Int support.The addition of the
math/bigpackage import is necessary for initializing the newMinMonthlyFeefield withbig.NewInt(0).
95-104: Updated Node structure with new NodesV2 fields.The test has been updated to include the new fields from the NodesV2 implementation:
IsReplicationEnabled,IsApiEnabled,IsDisabled, andMinMonthlyFee. These fields are essential for the enhanced node management capabilities described in the PR objectives.
237-246: Consistent field updates across test cases.The second test case also correctly initializes all the new fields in the Node structure, maintaining consistency with the first test case and ensuring proper test coverage for the updated node structure.
pkg/mocks/registry/mock_NodesContract.go (4)
9-9: Updated import to use NodesV2 package.The mock now correctly imports the
nodesv2package instead of the previous nodes package.
25-31: Method renamed from AllNodes to GetAllNodes.The method has been renamed from
AllNodestoGetAllNodesto match the changes in the interface, and the panic message has been updated accordingly.
33-44: Return type updated to use NodesV2 data structures.The return type has been correctly updated from
[]nodes.NodesNodeWithIdto[]nodesv2.INodesNodeWithId, and the type assertions and error handling have been adjusted accordingly.
55-81: Mock helper methods updated for the renamed method.All related helper methods and structs have been correctly updated to reflect the method name change from
AllNodestoGetAllNodes.
| if node.IsDisabled || !node.IsApiEnabled { | ||
| continue | ||
| } | ||
| nodeLocations[i] = uint32(i) * spacing |
There was a problem hiding this comment.
I see your conversation with the AI below about the extra for loop, but I think having empty indexes in this array is going to be a footgun that will make the code harder to reason about.
There was a problem hiding this comment.
It shouldn't apply here in the payer. The contract registry gets rid of those empty nodes during refreshData(), right?
newNodes := []Node{}
for _, node := range fromContract {
// nodes realistically start at 100, but the contract fills the array with empty nodes
if !node.IsValidConfig {
continue
}
There was a problem hiding this comment.
Maybe we're taking about different things. We have a nodeLocations array of length numNodes, but only the indexes with IsDisabled = false && IsApiEnabled = true have data in them. The rest of the indexes are set to 0 because of the continue on line 62.
There was a problem hiding this comment.
Oh, I see. Good catch! I just modified a bit the test TestGetNode_NoAvailableActiveNodes and triggered an error because of this. Modifying this.
There was a problem hiding this comment.
Modified. I moved the banlist filter up as well, now we filter earlier, sort, and return the candidate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/api/payer/nodeSelector_test.go (2)
75-75: Avoid exact string matching for error messagesAsserting on the exact error message text might make tests brittle. If the error message is updated slightly, the test will fail even if the behavior is correct.
Consider using string contains or regex matching to assert that the error message contains the essential information:
-require.Equal(t, "no available nodes after filtering", err.Error()) +require.Contains(t, err.Error(), "no available nodes")
78-93: Create a matrix of node health conditions to testThis test confirms that at least one healthy node is correctly returned, but it doesn't validate different combinations of node health properties (like a node with API disabled but not marked as completely disabled).
Consider extending the test cases to create a matrix that covers various node health configurations:
- Node with IsApiEnabled=false but other fields healthy
- Node with IsReplicationEnabled=false but other fields healthy
- Node with IsValidConfig=false but other fields healthy
This would provide more thorough coverage of the filtering logic introduced in this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/api/payer/nodeSelector.go(3 hunks)pkg/api/payer/nodeSelector_test.go(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/api/payer/nodeSelector.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test (Node)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (3)
pkg/api/payer/nodeSelector_test.go (3)
62-76: Test the different unhealthy conditions separatelyWhile this test verifies the behavior when all nodes are unhealthy, it doesn't specifically test different unhealthy conditions (e.g., API disabled vs replication disabled). Consider creating test cases that verify each specific filtering condition independently.
The implementation of
GetUnhealthyNodein the testutils package may only be settingIsDisabledto true while leaving other fields (likeIsApiEnabledorIsReplicationEnabled) unchanged. This test might not fully verify the PR's objective to skip nodes with API disabled.#!/bin/bash # Check the implementation of GetUnhealthyNode grep -r "func GetUnhealthyNode" --include="*.go" . cat $(grep -l "func GetUnhealthyNode" --include="*.go" .)
98-118: LGTM! Clear node configurationThe expanded node attributes in this test provide explicit control over all health-related fields, making the test intentions clearer.
136-244: Good refactoring using test utilitiesUsing
testutils.GetHealthyNode()across all test cases improves consistency and makes the tests more maintainable.
pkg/api/payer/nodeSelector.go
Outdated
| // Filter out banned, disabled, or non-API nodes | ||
| var availableNodes []registry.Node | ||
| for _, node := range nodes { | ||
| if node.IsDisabled || !node.IsApiEnabled { | ||
| continue | ||
| } |
There was a problem hiding this comment.
I don't think that this is the right place to do this. reg.GetNodes() already does a bunch of pre-filtering. Why dont we do it there so its all in one place? Look at func (s *SmartContractRegistry) refreshData() error
pkg/api/payer/nodeSelector.go
Outdated
| if node.IsDisabled || !node.IsApiEnabled { | ||
| continue | ||
| } | ||
| if _, exists := banned[node.NodeID]; !exists { |
There was a problem hiding this comment.
If the number of available nodes changes based on the ban list, then the partitions will change every time you add a member to the banlist and retry.
I don't think that works. The consistent hashing ring should be consistent between retries.
There was a problem hiding this comment.
What if we did something like this, so that the numNodes and spacing doesn't change unless the result set from the registry changes.
// GetNode selects a node for a given topic using stable hashing
func (s *StableHashingNodeSelectorAlgorithm) GetNode(
topic topic.Topic,
banlist ...[]uint32,
) (uint32, error) {
nodes, err := s.reg.GetNodes()
if err != nil {
return 0, err
}
// Flatten banlist
banned := make(map[uint32]struct{})
for _, list := range banlist {
for _, id := range list {
banned[id] = struct{}{}
}
}
// Sort availableNodes to ensure stability
sort.Slice(nodes, func(i, j int) bool {
return nodes[i].NodeID < nodes[j].NodeID
})
topicHash := HashKey(topic)
numNodes := uint32(len(nodes))
maxHashSpace := ^uint32(0)
spacing := maxHashSpace / numNodes
// Compute virtual positions for each available node
nodeLocations := make([]uint32, numNodes)
for i := range availableNodes {
nodeLocations[i] = uint32(i) * spacing
}
// Binary search to find the first node with a virtual position >= topicHash
idx := sort.Search(len(nodeLocations), func(i int) bool {
node := nodes[i]
if node.IsDisabled || !node.IsApiEnabled {
return false
}
if _, banned := banned[node.NodeID]; banned {
return false
}
return topicHash < nodeLocations[i]
})
// Select the appropriate node from availableNodes
candidateIdx := idx % len(nodeLocations)
return availableNodes[candidateIdx].NodeID, nil
}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
pkg/api/payer/nodeSelector_test.go (2)
1-283: 💡 Verification agent❓ Verification inconclusive
Verify the missing test cases mentioned in the AI summary.
The AI summary mentions two new test functions that should be in this file but aren't visible in the provided code:
TestGetNode_NoAvailableActiveNodesTestGetNode_AtLeastOneNodeIsAvailableThese test cases appear to be important for verifying the new node selection criteria mentioned in the PR objectives.
🏁 Script executed:
#!/bin/bash # Check if these test functions exist elsewhere in the file or in other test files grep -r "TestGetNode_NoAvailableActiveNodes\|TestGetNode_AtLeastOneNodeIsAvailable" --include="*.go" ./pkg/api/payer/Length of output: 119
Missing Required Test Cases
Our search indicates that the test functionsTestGetNode_NoAvailableActiveNodesandTestGetNode_AtLeastOneNodeIsAvailable—referenced in the AI summary and intended to verify the new node selection criteria—are not present anywhere in thepkg/api/payer/directory. Please add these tests to ensure that both edge cases (no available active nodes and at least one available node) are covered.
6-283: 💡 Verification agent🧩 Analysis chain
Missing tests for disabled node conditions.
According to the PR objectives, the payer node selection process has been updated to skip nodes that are disabled or don't have the API enabled. However, the current tests don't verify this functionality.
Consider adding test cases that verify the node selection behavior when some nodes are:
- Disabled by administrators (
IsDisabled: true)- Have API disabled (
IsApiEnabled: false)- Have replication disabled (
IsReplicationEnabled: false)You could add tests similar to the existing ones but include unhealthy nodes in the mix to verify they're properly skipped.
🏁 Script executed:
#!/bin/bash # Check if there's already a function for unhealthy nodes grep -r "func GetUnhealthyNode" --include="*.go" ./pkg/testutilsLength of output: 155
Missing Tests for Disabled/Unhealthy Node Conditions
The current tests in
pkg/api/payer/nodeSelector_test.godo not verify that the node selection algorithm properly skips nodes that are disabled, have API disabled, or have replication disabled—even though the PR objectives indicate these nodes must be ignored. Notably, we’ve confirmed via the existence ofGetUnhealthyNodeinpkg/testutils/contractRegistry.gothat there is support for simulating unhealthy nodes. However, none of the tests currently validate that nodes flagged withIsDisabled: true,IsApiEnabled: false, orIsReplicationEnabled: falseare skipped.Please add test cases to verify:
- Disabled Nodes: Ensure that when one or more nodes have
IsDisabled: true, the selector bypasses them.- API Disabled Nodes: Verify that nodes with
IsApiEnabled: falseare not chosen.- Replication Disabled Nodes: Confirm that nodes with
IsReplicationEnabled: falseare likewise skipped.Using helper functions like
testutils.GetUnhealthyNode(or similar functions for unhealthy configs) could simplify constructing these scenarios. This will ensure that the selection process behaves as intended under all conditions.
🧹 Nitpick comments (2)
pkg/api/payer/nodeSelector_test.go (2)
64-86: Consider using utility function for consistency.While other tests have been updated to use
testutils.GetHealthyNode(), this test still uses explicit node definitions. Consider updating it for consistency with the rest of the file.mockRegistry.On("GetNodes").Return([]registry.Node{ - { - NodeID: 100, - IsDisabled: false, - IsReplicationEnabled: true, - IsApiEnabled: true, - IsValidConfig: true, - }, - { - NodeID: 200, - IsDisabled: false, - IsReplicationEnabled: true, - IsApiEnabled: true, - IsValidConfig: true, - }, - { - NodeID: 300, - IsDisabled: false, - IsReplicationEnabled: true, - IsApiEnabled: true, - IsValidConfig: true, - }, + testutils.GetHealthyNode(100), + testutils.GetHealthyNode(200), + testutils.GetHealthyNode(300), }, nil)
183-195: Improve test performance with a more efficient approach.The brute force approach to find topics that hash into each bucket could be inefficient for large numbers of nodes. Consider using a more deterministic approach or reducing the maximum iterations.
- for i := 0; i < 1000000; i++ { + // Reduce maximum iterations to improve test performance + for i := 0; i < 100000; i++ { tpc := *topic.NewTopic(topic.TOPIC_KIND_IDENTITY_UPDATES_V1, []byte(strconv.Itoa(i))) node, err := selector.GetNode(tpc) require.NoError(t, err) if _, exists := targetBuckets[node]; !exists { targetBuckets[node] = tpc } // Stop early if all buckets are filled if len(targetBuckets) == len(nodes) { break } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/api/payer/nodeSelector.go(1 hunks)pkg/api/payer/nodeSelector_test.go(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/api/payer/nodeSelector.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Test (Node)
🔇 Additional comments (3)
pkg/api/payer/nodeSelector_test.go (3)
3-15: Import changes align with new dependency requirements.The addition of
strconvandgithub.com/xmtp/xmtpd/pkg/testutilsimports support the improvements made to the test suite, particularly the new utility for creating standardized node test instances.
19-25: Good use of utility functions for test readability.Replacing hardcoded node definitions with
testutils.GetHealthyNode()improves maintainability and readability by standardizing how healthy nodes are created across tests.
11-13:Details
❓ Verification inconclusive
Verify compatibility with the new NodesV2 package.
Since this PR is about integrating NodesV2, it's important to verify that these tests are compatible with the new package structure. Let's check for any imports or usage of the old nodes package vs. the new nodesv2 package.
🏁 Script executed:
#!/bin/bash # Check for imports of the old nodes package versus the new nodesv2 package grep -r "import.*\"github.com/xmtp/xmtpd/pkg/nodes\"" --include="*.go" ./pkg/api/payer/ grep -r "import.*\"github.com/xmtp/xmtpd/pkg/nodesv2\"" --include="*.go" ./pkg/api/payer/Length of output: 177
ACTION REQUIRED: Confirm NodesV2 Integration in Payer Tests
The automated search in the
pkg/api/payer/directory did not reveal any import statements for either the oldnodespackage or the newnodesv2package within the test file (nodeSelector_test.go). This might indicate that the tests are not directly interacting with either node package. However, given that the goal of this PR is to integrate NodesV2, please manually verify whether the intended functionality from NodesV2 is exercised elsewhere or through indirect dependencies. If these tests are expected to interact with NodesV2 functionality, additional modifications or test coverage might be required.
- Review the overall test suite: Ensure that NodesV2 functionality is properly validated in related test files or through integration tests.
- Confirm dependencies: Double-check that any indirect usage of node-related logic (e.g., via the
registryor mocks) are updated to reflect the NodesV2 integration.- Manual verification: Revisit the test configurations and runtime behavior to confirm that the absence of direct NodesV2 imports in this file does not compromise the verification of the new package integration.
| } | ||
|
|
||
| // Interface implementation guard. | ||
| var _ NodeRegistry = &SmartContractRegistry{} |
There was a problem hiding this comment.
ooh. I didn't know about this. Thanks for teaching me!
|
No objections on my end |
Summary -- - Closes #541 - CLI only uses the new Nodes ABI. - Deprecate unused testutils. - Migrator now migrates between V2 registries. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Streamlined CLI commands for node management by removing outdated health commands and introducing new commands for enabling/disabling nodes and adjusting configurations. - Introduced an optional minimum monthly fee during node registration. - Expanded administrative capabilities with new options to adjust commission, set active node limits, and update service endpoints. - Enhanced transaction execution for smoother blockchain operations. - Added new environment variables for improved node registration and management. - Introduced new test functions to validate node management functionalities. - **Refactor** - Consolidated node data structures and removed legacy versioning for simplified logic. - Updated configuration retrieval to ensure consistent contract addressing. - Transitioned to a unified nodes contract for improved management and access control. - **Tests** - Expanded test coverage to validate new node management functions and transaction handling. - Introduced tests for new functionalities related to node operations and configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
pkg/blockchain/client.go (1)
19-66: Well-structured helper for blockchain transaction executionThis new function consolidates common blockchain transaction patterns into a clean helper that handles transaction creation, waiting for mining, and log processing. This abstraction will improve code reuse and readability.
Consider logging errors from
eventParserrather than silently continuing on line 60:if err != nil { + logger.Debug("error parsing event log", zap.Error(err)) continue }pkg/blockchain/migrator/migrator_test.go (3)
62-62: Consider adding tests for persistent failures.
IfAddNoderepeatedly fails, the function only short-circuits after a timeout. A test to confirm behavior on invalid inputs or network errors could be helpful.
66-73: Add coverage for non-default field values.
The new fields—MinMonthlyFee,IsReplicationEnabled, andIsApiEnabled—are assigned defaults here. Consider also testing scenarios where these fields differ from their defaults.
106-106: Consider using the returned value for validation.
Registering a node without checking the returned struct might miss validation opportunities.pkg/blockchain/migrator/migrator.go (2)
32-32: Ensure error paths are fully tested.
The unmarshal step can fail on invalid pubkeys, so consider thorough negative tests.Also applies to: 37-37
74-74: Partial success is feasible.
If a subset of nodes fail mid-loop, it might partially update the registry. Clarify or document if needed.pkg/config/cliOptions.go (2)
13-17: NodeManagerOptions is well-defined.
Consider highlighting how it differs from AdminOptions in documentation.
18-22: "Enable" boolean might be clearer as "ShouldEnable."
Refinement can help if toggling states.pkg/blockchain/registryAdmin_test.go (2)
42-46: Helper function addRandomNode.
Nice DRY approach. Generating random addresses and a fixed minMonthlyFee aids in consistent coverage. If needed, consider verifying node creation with a read-back step.Also applies to: 49-50, 53-59, 62-62
140-146: TestSetMaxActiveNodes.
Checks for successful acceptance of new maximum. Consider adding a test for the revert scenario when current active nodes exceed the new limit.pkg/blockchain/registryAdmin.go (1)
377-402: SetMaxActiveNodes method.
Straightforward: updates a single on-chain value and logs the event. Consider any boundary checks if not enforced in the contract.contracts/src/Nodes.sol (1)
217-227: getAllNodes.
Returns an array of NodeWithId for enumerating all minted nodes. Consider indexing or pagination if the list grows large.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
cmd/cli/cli_test.go(3 hunks)cmd/cli/main.go(5 hunks)contracts/dev/deploy-local(1 hunks)contracts/dev/generate(1 hunks)contracts/dev/lib/common(2 hunks)contracts/script/DeployNodeRegistry.s.sol(2 hunks)contracts/script/DeployNodeRegistryV1.s.sol(0 hunks)contracts/src/Nodes.sol(1 hunks)contracts/src/NodesV2.sol(0 hunks)contracts/test/Nodes.sol(3 hunks)dev/baked/Dockerfile(1 hunks)dev/local.env(1 hunks)dev/register-local-node(1 hunks)dev/register-local-node-2(1 hunks)pkg/blockchain/client.go(2 hunks)pkg/blockchain/migrator/migrator.go(4 hunks)pkg/blockchain/migrator/migrator_test.go(4 hunks)pkg/blockchain/registryAdmin.go(2 hunks)pkg/blockchain/registryAdmin_test.go(3 hunks)pkg/blockchain/registryCaller.go(2 hunks)pkg/blockchain/registryCaller_test.go(1 hunks)pkg/config/cliOptions.go(1 hunks)pkg/mocks/registry/mock_NodesContract.go(2 hunks)pkg/registry/contractRegistry.go(4 hunks)pkg/registry/contractRegistry_test.go(5 hunks)pkg/registry/interface.go(1 hunks)pkg/testutils/config.go(1 hunks)pkg/testutils/contracts.go(1 hunks)
💤 Files with no reviewable changes (2)
- contracts/script/DeployNodeRegistryV1.s.sol
- contracts/src/NodesV2.sol
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/registry/interface.go
- cmd/cli/cli_test.go
- pkg/mocks/registry/mock_NodesContract.go
🔇 Additional comments (168)
pkg/testutils/contracts.go (1)
142-142: Function signature changed to use DeployNodes with updated parameters.The deployment of the Nodes contract now passes
auth.Fromas the third parameter tonodes.DeployNodes. This aligns with the PR's objective of integrating NodesV2 functionality into the main Nodes contract.pkg/testutils/config.go (1)
79-82: Updated NodesContractAddress to use XMTPNodeRegistry.json.The path for the Nodes contract configuration has been changed from "Nodes.json" to "XMTPNodeRegistry.json", which aligns with the consolidated contract approach mentioned in the PR objectives.
contracts/dev/lib/common (4)
35-35: Updated nodes deployment to use a script-based approach.The deployment command for Nodes has been changed to use
forge scriptwith theDeployNodeRegistry.s.solscript rather than the previous approach. This standardizes deployment with other contracts that also use scripts.
37-38: Updated error message to reflect the new contract name.The error message has been modified to simply refer to "Nodes contract" rather than including any version number, which aligns with the standardization effort in this PR.
41-41: Updated success message and output path.The success message now points to "XMTPNodeRegistry.json" instead of a version-specific file, which matches the changes made in the config.go file and creates consistency across the codebase.
56-56: Updated help text to remove nodes_v2 option.The list of valid options has been updated to remove 'nodes_v2', reflecting the consolidation of node contracts indicated in the PR objectives.
contracts/dev/generate (1)
54-54: Simplified contract list by removing NodesV2.The default list of contracts for binding generation has been updated to remove "NodesV2" and reorder the remaining contracts. This is consistent with the PR's objective to integrate NodesV2 functionality into the main Nodes contract.
dev/register-local-node-2 (5)
9-9: Properly exporting the NODE_OPERATOR_PRIVATE_KEYAdded the necessary environment variable for node operations, using the existing ANVIL_ACC_2_PRIVATE_KEY. This aligns with the NodesV2 integration requirements.
11-15: Updated node registration command with proper flag formatThe command now correctly uses the
--admin.private-keyflag format (instead of--admin-private-key) and captures the node ID output usingjq. This matches the updated CLI argument structure.
17-20: New node API enabling functionalityAdded command to enable the API for the registered node using the operator's private key. This implements the NodesV2 feature where nodes can have their API explicitly enabled/disabled.
22-25: New node replication enabling functionalityAdded command to enable replication for the registered node, addressing the PR objective of controlling replication at the node level.
27-27: Added clear success messageGood addition of a colored success message that confirms the node ID and successful enabling of features.
dev/baked/Dockerfile (1)
32-32: Updated contract address source to XMTPNodeRegistryThe environment variable
XMTPD_CONTRACTS_NODES_ADDRESSnow correctly references the new contract structure fromXMTPNodeRegistry.jsoninstead of the previousNodes.json. This aligns with the migration to NodesV2.contracts/dev/deploy-local (1)
32-32: Simplified nodes deployment commandThe command for deploying nodes has been simplified to
forge_deploy_script nodes, removing the explicit source file and contract name parameters. This indicates the deployment logic has been moved to the script itself, aligning with the NodesV2 integration.dev/register-local-node (5)
8-8: Properly exporting the NODE_OPERATOR_PRIVATE_KEYAdded the necessary environment variable for node operations, using the existing ANVIL_ACC_1_PRIVATE_KEY. This aligns with the NodesV2 integration requirements.
10-14: Updated node registration command with proper flag formatThe command now correctly uses the
--admin.private-keyflag format (instead of--admin-private-key) and captures the node ID output usingjq. This matches the updated CLI argument structure.
16-20: New node API enabling functionalityAdded command to enable the API for the registered node using the operator's private key. This implements the NodesV2 feature where nodes can have their API explicitly enabled/disabled by NOPs, as mentioned in the PR objectives.
21-25: New node replication enabling functionalityAdded command to enable replication for the registered node, directly implementing the PR objective of allowing NOPs to control replication at the node level.
26-26: Added clear success messageGood addition of a colored success message that confirms the node ID and successful enabling of features.
pkg/blockchain/registryCaller_test.go (4)
9-18: Test for GetAllNodes looks good.The test successfully validates that the
GetAllNodesmethod retrieves nodes correctly, checking that no errors occur and that the expected node count is returned.
20-29: Test for GetNode looks good.The test correctly verifies that
GetNoderetrieves a specific node by ID and performs appropriate assertions to validate the result.
31-37: Test for GetNodeNotFound looks good.This test properly validates the error handling for the case when a node with a non-existent ID is requested.
39-47: Test for OwnerOf looks good.The test effectively verifies that the
OwnerOfmethod executes without errors after adding a random node.contracts/script/DeployNodeRegistry.s.sol (3)
8-8: Updated import to Nodes.sol.The import statement has been updated to reference the new
Nodes.solfile instead ofNodesV2.sol, aligning with the PR objective to integrate NodesV2.
11-11: Updated variable type from NodesV2 to Nodes.The variable type has been updated to match the new contract name.
26-26: Updated contract instantiation from NodesV2 to Nodes.The contract instantiation has been updated to use the new
Nodescontract. This change is consistent with the import and variable type changes.contracts/test/Nodes.sol (3)
7-7: Updated import to Nodes.sol.The import statement has been properly updated to reference the new
Nodes.solfile, consistent with other contract reference updates in the codebase.
17-17: Updated variable type from NodesV2 to Nodes.The variable type in the test contract has been updated to use the new
Nodescontract.
31-31: Updated contract instantiation from NodesV2 to Nodes.The contract instantiation has been updated to use the new
Nodescontract. The tests cover a wide range of functionality including node management, enabling/disabling, API and replication management, which aligns with the PR objectives to support skipping disabled nodes.dev/local.env (1)
13-13: Updated node address retrieval.The environment variable assignment has been updated to extract the node registry address from a different location in the JSON file structure, reflecting the changes in contract deployment. This aligns with the move from
NodesV2toNodes.pkg/registry/contractRegistry_test.go (3)
49-71: Properly updated to use new V2 node interface and additional flagsThe test has been correctly updated to use the new
GetAllNodesmethod and new structure withINodesNodeWithId. The addition of the new fields (IsDisabled,IsApiEnabled,IsReplicationEnabled) aligns with the NodesV2 integration.
104-124: Updated method signature and node field structure correctlyThis test has been properly updated to match the new signature and fields from NodesV2 integration. The test logic maintains its original purpose while accommodating the new structure.
159-173: Properly updated to work with NodesV2 interfaceThe test now correctly uses the NodesV2 interface with appropriate field structure including the added
IsDisabled,IsApiEnabled, andIsReplicationEnabledflags that are central to this integration.pkg/registry/contractRegistry.go (5)
50-51: Excellent addition of interface implementation guardThis helps catch potential interface implementation issues at compile time, ensuring the
SmartContractRegistryproperly implements theNodeRegistryinterface.
219-219: Updated to use new GetAllNodes method correctlyThe method call has been properly updated from
AllNodestoGetAllNodesto align with the NodesV2 contract interface.
235-235: Updated parameter type to use the new interfaceThe function signature has been properly updated to use
nodes.INodesNodeWithIdinstead of the previous type, reflecting the V2 contract structure.
249-251: Now correctly handles disabled nodesThis change aligns with the PR objective to skip nodes that are disabled by administrators. Setting
isValidConfigto false ensures these nodes won't be selected for operations.
254-262: Node struct fields properly expanded with V2 propertiesThe Node struct now includes the expanded capabilities from NodesV2 -
IsReplicationEnabled,IsApiEnabled,IsDisabled, andMinMonthlyFee, which enables the filtering functionality described in the PR objectives.pkg/blockchain/registryCaller.go (3)
15-21: Interface updated to support NodesV2 functionalityThe interface now properly supports the NodesV2 contract with more specific methods that align with the PR objectives, particularly around node filtering based on enabled/disabled status.
23-27: Simplified caller structureThe structure was renamed and simplified to better reflect its purpose in the updated system.
49-63: Added support for filtering active nodesThese new methods implement the ability to filter nodes based on API and replication status, which directly supports the PR objective to skip nodes that are disabled or don't have API enabled.
pkg/blockchain/migrator/migrator_test.go (12)
22-22: No concerns with the updated contract address.
57-57: Unified return type is consistent with the new design.
77-77: No issues with the registry setup call.
80-81: Good approach for multiple-node testing.
83-83: Reading from the registry is straightforward.
90-92: Adequate assertions for new fields.
108-108: Simple read action looks good.
129-130: Consistent registration for additional nodes.
132-132: Reading the newly added nodes is fine.
136-138: Effective use of a second registry for thorough testing.
141-141: Verifying data consistency across registries.
154-157: Checks default values thoroughly.pkg/blockchain/migrator/migrator.go (12)
14-14: Consolidating multiple node versions into one struct reduces duplication.
19-19: Switch to int64 for MinMonthlyFee is more straightforward for arithmetic.
24-29: Clear early return of errors in ReadFromRegistry.
30-31: Pre-sizing the slice is efficient and readable.
42-50: Assigning the new fields aligns well with the consolidated struct.
53-53: Straightforward return of the constructed list.
56-56: Iteration logic for WriteToRegistry is clear and consistent.Also applies to: 58-59
63-63: Context usage per node addition is consistent.
69-69: Directly mapping SerializableNode fields into AddNode keeps things simple.
84-84: DumpNodesToFile uses defer effectively to close the resource.
94-95: ImportNodesFromFile mirrors DumpNodesToFile.
101-101: Decoding into []SerializableNode is straightforward.pkg/config/cliOptions.go (9)
8-11: Grouping admin info in AdminOptions clarifies usage.
30-30: Empty GetOptions suggests you may extend it later.
33-34: Replacing AdminPrivateKey with AdminOptions is consistent.
37-38: GetPubKeyOptions is clearly scoped for private key conversions.
41-47: RegisterNodeOptions comprehensively captures required fields.
49-51: Verify correct usage of NodeManagerOptions in set-http-address command.
Its NodeId must match the same struct, not a separate one.
54-57: SetMinMonthlyFeeOptions introduced similarly.
Check that the CLI references the correct NodeId field.
59-62: SetMaxActiveNodesOptions reuses AdminOptions seamlessly.
64-67: SetNodeOperatorCommissionPercentOptions reuses AdminOptions as well.cmd/cli/main.go (24)
24-38: New fields in the CLI struct provide structured subcommand handling.
51-64: Declaring variables for each option group is consistent with the new approach.
66-72: Admin commands are properly grouped and named.
74-74: Register-node and migrate-nodes commands align with the new node management design.Also applies to: 76-77
79-90: disable/enable-node and removal from API/replication nodes.
Implementation is consistent with AdminOptions usage.
91-105: Set commands for node configuration.
Ensure each command references the correct NodeId from its respective struct.
107-126: Commands for NodeOperatorOptions are well integrated.
129-137: Error message for missing command is clear.
142-155: Assembling the final CLI object with new subcommands looks solid.
164-188: Key management commands are straightforward.
190-230: RegisterNode checks negative fees and uses the new structure effectively.
232-252: disableNode logic is consistent and straightforward.
254-274: enableNode mirrors disableNode with no issues.
276-318: Node removal commands are aligned with the registry admin interface.
320-342: MigrateNodes uses import and write, providing robust migration flow.
344-364: Max active nodes can be configured easily, good usage of AdminOptions.
366-386: setNodeOperatorCommissionPercent is implemented as expected.
450-471: setApiEnabled uses NodeOperatorOptions consistently.
473-494: setReplicationEnabled also follows the same operator pattern.
502-528: getActiveApiNodes retrieves and prints the results without issues.
530-556: getActiveReplicationNodes usage is consistent with getActiveApiNodes.
558-591: getAllNodes fetches and optionally dumps the list of nodes to file.
593-618: getNode references NodeOperator.NodeId properly.
704-736: setupRegistryAdmin is a clear single point for registry construction.pkg/blockchain/registryAdmin_test.go (12)
14-14: Use of multiple return values is clear and convenient.
Returning both admin and caller interfaces, plus context and cleanup, keeps the tests self-contained and easy to manage.
19-20: Consistent contract deployment for deterministic tests.
Always deploying the nodes contract ensures reproducible behavior across tests. However, be mindful of test runtime if the deployment process is heavy.
31-34: Properly initializing and returning registry and caller.
Good job creating both interfaces and verifying errors. This pattern helps reduce boilerplate in individual test functions.Also applies to: 37-37
64-70: TestAddNode coverage.
This test verifies successful node creation with a valid owner, random signing key, and typical minMonthlyFee. Looks comprehensive.Also applies to: 73-73
83-87: TestAddNodeBadOwner scenario.
Good negative test ensuring invalid owner addresses are rejected.
89-98: TestAddNodeBadMinMonthlyFee.
Validates negative fee handling. This confirms robust input validation.
100-108: TestDisableNode correctness.
Verifies that disabling the node by ID triggers no error. Consider confirming the node’s state post-disabling if relevant.
110-118: TestEnableNode correctness.
Re-enables a previously added node. Straightforward coverage. Similar to disable, consider asserting final node state.
120-128: TestRemoveFromApiNodes.
Ensures removing a node from API list is error-free. Good to see coverage for partial deactivation scenarios.
130-138: TestRemoveFromReplicationNodes.
Similar structure to removing from API nodes. The additional coverage for replication ensures consistent node-state transitions.
148-154: TestSetNodeOperatorCommissionPercent coverage.
Verifies a valid commission percent is set. Straightforward test to ensure on-chain parameter acceptance.
156-162: TestSetNodeOperatorCommissionPercentInvalid scenario.
Ensures negative or out-of-range commission values are rejected. Good boundary testing.pkg/blockchain/registryAdmin.go (17)
12-12: Added import for types.
Inclusion of core/types is standard for event parsing under Geth. No concerns.
26-37: Extended INodeRegistryAdmin interface.
New methods (DisableNode, EnableNode, etc.) align with requirements for node management. All appear consistent with the Nodes contract changes.
40-45: nodeRegistryAdmin struct.
Defines essential fields (client, signer, logger, contract). Straightforward composition for a registry admin utility.Also applies to: 47-47
49-49: Interface compliance check.
Usingvar _ INodeRegistryAdmin = &nodeRegistryAdmin{}is a clean way to ensure all interface methods are implemented.
54-54: NewNodeRegistryAdmin signature.
Factory-style constructor returning*nodeRegistryAdminis clear and typical in Go.
63-66: Allocating logger and contract references.
Well-organized. The logger name “NodeRegistryAdmin” helps with targeted logs.
72-72: AddNode method signature and input validation.
Including a non-negative minMonthlyFee check is a good defensive measure.Also applies to: 76-76, 82-84
93-123: ExecuteTransaction usage for AddNode.
Nice structured approach: binding the transaction, parsing events, and logging. This pattern is consistent and readable.
127-150: DisableNode transaction.
Flow parallels AddNode, with appropriate event parsing and logging. Straightforward logic.
152-175: EnableNode transaction.
Same transaction pattern, event parse, and logging. No immediate concerns.
177-202: RemoveFromApiNodes method.
Consistent approach to remove from a specialized list, ensuring correct event (ApiDisabled) is parsed.
204-229: RemoveFromReplicationNodes method.
Same pattern as RemoveFromApiNodes. Thorough coverage of replication logic.
231-265: SetHttpAddress method.
Clear, with an event-based callback to confirm the update.
267-301: SetMinMonthlyFee method.
Ensures a transaction with minMonthlyFee updated for the node. Proper event parse with MinMonthlyFeeUpdated.
303-336: SetIsApiEnabled method.
Conditional parsing of ApiEnabled or ApiDisabled is carefully handled. Good approach to unify the method.
338-375: SetIsReplicationEnabled method.
Matches the pattern from SetIsApiEnabled. Clean and symmetrical.
404-443: SetNodeOperatorCommissionPercent method.
Verifies commissionPercent range, executes transaction, logs an event. Good boundary validation.contracts/src/Nodes.sol (41)
5-7: New imports for AccessControl and EnumerableSet.
They align with the updated design for role-based access and set management.
9-21: Updated doc comments.
Makes it clear what the contract does and how node ownership is managed. Good thorough explanation.
22-29: Role definitions and constants.
ADMIN_ROLE, NODE_MANAGER_ROLE, and MAX_BPS stand out for clarity, enabling flexible role-based permissions.
30-30: NODE_INCREMENT usage.
Incrementing IDs by 100 is unique but presumably intentional for distinct node ranges.
33-34: _baseTokenURI and maxActiveNodes definitions.
Defining a small default maxActiveNodes (20) is reasonable for initial constraints.Also applies to: 36-38
40-43: _nodeCounter initialization.
Node IDs start at 100, stored as a multiple of NODE_INCREMENT. Straightforward logic.
48-54: Node storage fields and sets.
Keeping track of isDisabled, isApiEnabled, isReplicationEnabled, and minMonthlyFee offers fine-grained node state management.
55-59: nodeOperatorCommissionPercent.
Using basis points (BPS) is standard for percentage math. The upper limit of 10000 BPS is enforced later.
61-73: Constructor with AccessControlDefaultAdminRules.
Requires a non-zero _initialAdmin, grants roles, and sets up default admin logic. Thorough initialization.
75-77: Reserved admin-only comment section.
Clear delineation of privileges helps code organization.
79-96: addNode function.
Checks valid addresses and assigns Node struct. Ensures each new node is minted as an NFT. Straightforward event emission.
98-109: disableNode function.
Marking node as disabled, removing from active sets. Balanced approach for disabling.
111-119: removeFromApiNodes / removeFromReplicationNodes.
Additional granularity, removing the node from only API or replication sets.
121-130: enableNode function.
ClearsisDisabled. Straight to the point, includes an event.
132-139: setMaxActiveNodes.
Reverts if new max is lower than current active sets. Good protective measure.
141-146: setNodeOperatorCommissionPercent.
Properly ensures newCommissionPercent <= MAX_BPS. Great boundary logic.
148-155: setBaseURI.
Checks for trailing slash and non-empty string. Thoughtful validation.
160-176: transferFrom override with node disabling.
Ensuring node is deactivated before ownership transfer is a prudent design choice, requiring an explicit re-enable.
178-184: setHttpAddress.
Updates the node’s HTTP address with basic validation.
186-191: setMinMonthlyFee.
Allows adjusting the fee post-mint. Straightforward with event emission.
197-205: setIsApiEnabled.
Calls internal activation/deactivation functions. Only node owner can invoke it, ensuring correct permissions.
207-215: setIsReplicationEnabled.
Follows the same pattern as API enabling. Consistent approach to different node features.
229-230: getAllNodesCount.
Exposes _nodeCounter for external usage. Straightforward.
234-238: getNode.
Requires node to exist, then returns the struct.
240-250: getActiveApiNodes.
Enumerates API-enabled nodes from the set. Clear approach.
252-255: getActiveApiNodesIDs.
Returns the array of node IDs from the set. Concise.
257-260: getActiveApiNodesCount.
The set length is a direct measure of active API nodes.
262-265: getApiNodeIsActive.
Uses .contains(...) on the set to determine if the node is active.
268-278: getActiveReplicationNodes.
Mirrors the logic of getActiveApiNodes.
280-283: getActiveReplicationNodesIDs.
Same approach for replication.
284-287: getActiveReplicationNodesCount.
Again parallels the API nodes pattern.
289-292: getReplicationNodeIsActive.
Checks membership in replication set via .contains(...).
294-298: getNodeOperatorCommissionPercent.
Exposes stored BPS value. Clear read method.
300-304: Modifier whenNotDisabled.
Guards usage if node is flagged disabled. This enforces preconditions effectively.
306-311: _nodeExists internal check.
Uses _ownerOf(...) to confirm if the token is minted.
313-316: _baseURI override.
Retrieves the stored base token URI for ERC721 metadata.
318-327: _activateApiNode internal helper.
Requires that active nodes remain below maxActiveNodes. Good for ensuring capacity constraints.
329-337: _disableApiNode internal helper.
Removes the node from the API set and emits ApiDisabled. Straightforward.
339-348: _activateReplicationNode internal helper.
Same approach for replication. Good consistency.
350-358: _disableReplicationNode internal helper.
Removes node from replication set, logging an event.
360-369: supportsInterface override.
Centralizes bridging for multiple inherited interfaces.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
cmd/cli/main.go (4)
26-41: Add docstrings for new CLI fields for clarity.Lines 26-41 introduce multiple new fields in the
CLIstruct (e.g.,AdminOptions,NodeManagerOptions, etc.). While this is a good organizational improvement, consider adding short docstrings to each of these fields to provide clarity on which commands or use cases they apply to. This helps later contributors quickly understand the purpose and required usage of each field.
60-60: Ensure consistent naming.
setHttpAddressOptionsis clearly named. Just as a cautionary note, keep naming patterns consistent across the codebase (e.g., “Set…” vs. “set…” for variables).
62-66: Consider grouping new options.These lines add diverse new option structs (
nodeOperatorOptions,setMinMonthlyFeeOptions, etc.). While they are logically grouped in the CLI, consider grouping them in the code (or in separate files) if additional complexity grows in the future. This keeps the CLI parser easier to maintain.
758-790: Consider returning errors instead of callinglogger.Fatal.Currently,
setupRegistryAdminuseslogger.Fatalon errors, which will terminate the process immediately. Depending on usage, returning errors and allowing the caller to decide how to handle them can provide more flexibility (e.g., retry, fallback). Evaluate whether termination is always desired.pkg/config/cliOptions.go (4)
37-39: Mark as sensitive if needed.
GetPubKeyOptions.PrivateKeyis labeled “required,” but remember that private keys are sensitive. Make sure to handle them carefully, as recommended in other areas of the code.
54-59: Use clear naming for node ownership fields.The
RegisterNodeOptionsfields (HttpAddress,OwnerAddress,SigningKeyPub,MinMonthlyFee) are generally clear. As usage grows, consider docstrings or comments clarifying thatOwnerAddressis the final on-chain address holding the NFT.
67-71: Ensure no negative fees.
SetMinMonthlyFeeOptionshas anint64fee. While your code blocks negative values, consider adding a meaningful usage message or constraints in the flag definition so that the CLI user immediately knows negatives are invalid.
72-75: Clarify safe ranges for max active nodes.
SetMaxActiveNodesOptionssets auint8. Consider clarifying the safe range for your system capacity in docs or usage messages to avoid confusion if a user attempts an out-of-range value (e.g.,> 255).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/cli/main.go(5 hunks)pkg/config/cliOptions.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Test (Node)
🔇 Additional comments (8)
cmd/cli/main.go (3)
54-55: Initialize all option structs consistently.Initializing
adminOptionsandnodeManagerOptionshere is consistent, but double-check that each newly introduced struct is properly grouped or initialized in similar fashion for future maintainability and clarity.
445-466: Potential mismatch in node ID usage. (Previously noted)This code calls
setupRegistryAdminwithoptions.SetHttpAddress.NodeManagerOptions.NodePrivateKey(line 450) but usesoptions.NodeManagerOptions.NodeId(line 460). If these IDs differ, it can cause incorrect usage.
468-493: Likely the same mismatch for setMinMonthlyFee. (Previously noted)Here,
setupRegistryAdminis called withoptions.SetMinMonthlyFee.NodeManagerOptions.NodePrivateKey(line 473) whereasSetMinMonthlyFeeusesoptions.NodeManagerOptions.NodeId(line 487). If there is a mismatch in node IDs, it may lead to unexpected behavior.pkg/config/cliOptions.go (5)
8-22: Validate required fields for each admin-related operation.These new structs (
AdminOptions,NodeManagerOptions, andNodeOperatorOptions) define private keys and node IDs. Ensure that commands that rely on these fields enforce correctness at runtime, especially ifNodeIdis mandatory.
30-31: Empty struct is a good placeholder.
GetOptionsis an empty struct here, which is fine if you plan to add more fields later. No issues noted.
33-35: Check file existence earlier.
MigrateNodesOptionsincludes an input file (InFile). In calling code, consider verifying file readability earlier to provide more immediate feedback if the file does not exist.
62-65: Enforce or validate new addresses.
SetHttpAddressOptionsholds an address string. If an invalid address can cause issues downstream, add validations or pre-check logic.
77-80: Enforce commission percent boundaries.
SetNodeOperatorCommissionPercentOptionsuses anint64. Implementation should ensure it remains within acceptable limits (e.g., 0–100) to prevent invalid or negative commission.
Summary
NodesV2. This is a breaking change! Upgrade test is expected to fail!.Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
New Features
Tests