Updates Nodes CLI #591
Updates Nodes CLI #591
Conversation
WalkthroughThis pull request refactors the node management and command structure. It removes deprecated update commands from the CLI and reorders the command registration flow. The changes update the blockchain registry admin by removing versioning, adding new node management methods, and revising the Changes
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) 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
🧹 Nitpick comments (7)
pkg/blockchain/registryAdmin_test.go (3)
37-57: New helper functionaddRandomNode
Nice utility for setting up a node with default parameters. Consider parameterizing theminMonthlyFeefor more flexible testing in diverse scenarios.
59-71:TestAddNodeduplicates some logic inaddRandomNode
The direct call here is almost identical to the helper function approach. For consistency, you might reuseaddRandomNodeto keep test setup DRY.-func TestAddNode(t *testing.T) { - registry, ctx, cleanup := buildRegistry(t) - defer cleanup() - privateKey := testutils.RandomPrivateKey(t) - httpAddress := testutils.RandomString(32) - owner := testutils.RandomAddress() - require.Eventually(t, func() bool { - err := registry.AddNode(ctx, owner.String(), &privateKey.PublicKey, httpAddress, 1000) - return err == nil - }, 1*time.Second, 50*time.Millisecond) -} +func TestAddNode(t *testing.T) { + registry, ctx, cleanup := buildRegistry(t) + defer cleanup() + addRandomNode(t, registry, ctx) +}
159-164:TestSetBaseURI: sets the URI
Valid test, although you might also verify correctness (e.g., stored URI retrieval and verification).pkg/blockchain/registryAdmin.go (4)
21-36: ExpandedINodeRegistryAdmininterface
Providing methods likeDisableNode,EnableNode, andSetBaseURIfosters comprehensive node management. Ensure consistent parameter validation in each method (like inAddNode).
69-116:AddNodemethod
- Negative minMonthlyFee returns an error—a good validation.
- If
owneris invalid, an error is thrown.- Summarily, logic is sound. Could consider an additional check for empty
httpAddress.
154-170:RemoveFromApiNodes: usesRemoveFromApiNodescontract call
Implementation is correct. Might consider verifying the node’s initial state in tests before removal.
240-256:SetBaseURI: sets a string parameter
Implementation is straightforward. Consider trimming trailing spaces or slashes in a future improvement.
🛑 Comments failed to post (8)
pkg/registry/interface.go (2)
13-13: 💡 Verification agent
🧩 Analysis chain
Interface method signature update
The method has been renamed from
AllNodestoGetAllNodesand its return type changed to use the newnodesv2package. This is a breaking change that requires updates to all implementations of this interface.
🏁 Script executed:
#!/bin/bash # Check for any remaining calls to AllNodes that haven't been updated rg "AllNodes\(" --type go # Check implementations of the NodesContract interface ast-grep --pattern $'type \$_ struct { $$$ } func \($_\) GetAllNodes\(opts \*bind.CallOpts\) \(\[]nodesv2.INodesNodeWithId, error\) { $$$ }'Length of output: 2570
Action Required: Update AllNodes to GetAllNodes Across Implementations
The interface in
pkg/registry/interface.gohas been correctly updated to useGetAllNodeswith the new return type. However, the verification revealed several remaining references to the oldAllNodesmethod (for example, inpkg/blockchain/registryCaller.goand multiple functions incontracts/pkg/nodes/Nodes.go). Since these implementations no longer satisfy the new interface signature, please update all such occurrences accordingly.
- Ensure that all implementations in
contracts/pkg/nodes/Nodes.goare renamed fromAllNodestoGetAllNodesand adjust the return types as needed.- Verify that all call sites (e.g., in
pkg/blockchain/registryCaller.go) consistently useGetAllNodes.
5-5: 💡 Verification agent
🧩 Analysis chain
Package import update
This change shifts from using the
nodespackage to thenodesv2package, which aligns with the method signature changes below.Ensure that all imports throughout the codebase are consistently updated where this package is used.
🏁 Script executed:
#!/bin/bash # Check for any remaining imports of the old nodes package rg -l "github.com/xmtp/xmtpd/contracts/pkg/nodes" --type goLength of output: 289
Attention: Incomplete Package Import Update
You've updated the import in
pkg/registry/interface.goto usenodesv2, which aligns with the updated method signatures. However, our search indicates that the oldnodespackage is still imported in several files. Please update the import in the following files to usegithub.com/xmtp/xmtpd/contracts/pkg/nodesv2for consistency:
pkg/blockchain/registryAdmin.gopkg/blockchain/registryCaller.gopkg/mocks/registry/mock_NodesContract.gopkg/registry/contractRegistry_test.gopkg/registry/contractRegistry.gopkg/testutils/contracts.goOnce these updates are made, verify that all references to the old package are removed. Thank you!
pkg/registry/node.go (2)
17-25: 💡 Verification agent
🧩 Analysis chain
Enhanced Node struct with more detailed status fields
The Node struct has been expanded with more granular status fields:
IsReplicationEnabled- Controls if the node participates in replicationIsApiEnabled- Controls if the node's API endpoints are activeIsDisabled- Global flag to disable the nodeMinMonthlyFee- Specified as a big.Int for accurate fee calculationThis replaces the previous simpler
IsHealthyboolean, providing more fine-grained control over node behavior.
🏁 Script executed:
#!/bin/bash # Check for places that might still be using the removed IsHealthy field rg "\.IsHealthy" --type go # Check if there are Node initializations that need to set the new fields ast-grep --pattern 'Node{ $$$ }'Length of output: 1100
Action: Update legacy references to the removed
IsHealthyfield.The Node struct now features the new status fields (
IsReplicationEnabled,IsApiEnabled,IsDisabled, andMinMonthlyFee) to provide more granular control. However, our verification found that the legacyIsHealthyfield is still being referenced in the following locations:
- pkg/blockchain/migrator/migrator.go
- Line where
IsHealthy: node.Node.IsHealthyis still used.- pkg/blockchain/migrator/migrator_test.go
- Lines comparing
node1.IsHealthyandnode2.IsHealthywith other entries.Please update these instances to use the appropriate new status fields, ensuring that the legacy
IsHealthyfield is fully removed from the codebase.
38-42: 🛠️ Refactor suggestion
Updated Equals method to compare new fields
The
Equalsmethod has been properly updated to compare the new fields, ensuring correct equality testing.One potential issue: If either
MinMonthlyFeeis nil, the.Cmp()method call could lead to a nil pointer panic. Make sureMinMonthlyFeeis always initialized when creating a Node.Add nil check for MinMonthlyFee to avoid potential nil pointer panics:
- n.MinMonthlyFee.Cmp(other.MinMonthlyFee) == 0 && + ((n.MinMonthlyFee == nil && other.MinMonthlyFee == nil) || + (n.MinMonthlyFee != nil && other.MinMonthlyFee != nil && n.MinMonthlyFee.Cmp(other.MinMonthlyFee) == 0)) &&📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.n.IsReplicationEnabled == other.IsReplicationEnabled && n.IsApiEnabled == other.IsApiEnabled && n.IsDisabled == other.IsDisabled && ((n.MinMonthlyFee == nil && other.MinMonthlyFee == nil) || (n.MinMonthlyFee != nil && other.MinMonthlyFee != nil && n.MinMonthlyFee.Cmp(other.MinMonthlyFee) == 0)) && n.IsValidConfig == other.IsValidConfigpkg/blockchain/registryAdmin_test.go (4)
95-103:
⚠️ Potential issue
TestDisableNodeuses hardcoded node ID
Since node IDs are assigned sequentially in the contract, there's no guarantee the node will have ID 100. This test may give a false pass or fail unexpectedly. Consider capturing the newly added node’s actual ID from the event logs or from a contract query.
105-113:
⚠️ Potential issue
TestEnableNode: same hardcoded node ID concern
Similar toTestDisableNode, using a fixed ID is brittle. Retrieve the actual ID from contract events or queries before enabling it.
115-123:
⚠️ Potential issue
TestRemoveFromApiNodeswith a fixed ID
This test also needs a dynamic node ID approach. It’s safer to fetch the node ID from the actual add event results.
125-133:
⚠️ Potential issue
TestRemoveFromReplicationNodeswith a fixed ID
Same issue as above. Avoid relying on 100 as the node ID.
fbac
force-pushed
the
04-05-update_nodes_cli
branch
from
a9ff722 to
bcfc232
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (8)
cmd/cli/main.go (3)
56-58: Inconsistent error message.
The command is "migrate-nodes," but the error message can be misread as "dump nodes command." Consider updating it for clarity:- return nil, fmt.Errorf("could not add dump nodes command: %s", err) + return nil, fmt.Errorf("could not add migrate-nodes command: %s", err)
115-127: Consider avoiding logger.Fatal for improved error handling.
Usinglogger.Fatalterminates the application, which might limit testability or graceful shutdown. You may consider returning an error or using a warning/log + exit strategy explicitly in main.
257-260: Consider the potential size of logged data.
Logging the entire list of nodes could be large in production. You might want to log just the node count to avoid clutter or potential PII exposure.pkg/blockchain/client.go (1)
19-66: Event parsing silently ignores parse errors.
TheExecuteTransactionfunction is well-structured. However, parse errors are swallowed (continue) with no warning or logging. Consider logging or tracking these parse failures for debugging.- if err != nil { - continue - } + if err != nil { + logger.Warn("event parsing error", zap.Error(err)) + continue + }pkg/config/cliOptions.go (1)
8-10: Proposal: Consider environment-based approach for the admin private key
Storing a private key via CLI flags can be risky and might accidentally leak in logs or process snapshots. Consider environment variables, config files with restricted permissions, or other secure storage mechanisms.pkg/blockchain/registryCaller.go (2)
54-54: Reusing logic
GetActiveReplicationNodesmirrorsGetActiveApiNodes. If more similar methods appear, consider refactoring for code reuse.Also applies to: 57-59
62-62: Potential performance concern
Fetching all nodes at once may become inefficient if the registry grows large. Batching or pagination could improve scalability.Also applies to: 64-67
pkg/blockchain/migrator/migrator.go (1)
24-53: Careful with large node sets
Looping through and converting all nodes can be slow if the registry is large. Consider concurrency or pagination. Also note any unexpected data, such as invalid fees.
🛑 Comments failed to post (2)
cmd/cli/main.go (2)
72-73:
⚠️ Potential issueDuplicate addCommand call detected.
The "migrate-nodes" command is being added twice (lines 56-58 and 71-73). This duplication may lead to conflicts or unexpected behavior. Remove one instance to maintain a single registration.- if _, err := parser.AddCommand("migrate-nodes", "Migrate nodes from a file", "", &migrateNodesOptions); err != nil { - return nil, fmt.Errorf("could not add migrate-nodes command: %s", err) - }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
194-198:
⚠️ Potential issueFunction migrateNodes is never invoked.
Static analysis indicates this function is “unused.” If you intend to support a “migrate-nodes” command, add it to main’s switch; otherwise, remove the function.case "migrate-nodes": + migrateNodes(logger, options) return🧰 Tools
🪛 GitHub Check: Lint-Go
[failure] 194-194:
funcmigrateNodesis unused (unused)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/cli/main.go(5 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(1 hunks)pkg/blockchain/registryCaller.go(1 hunks)pkg/config/cliOptions.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/blockchain/client.go
- pkg/config/cliOptions.go
🧰 Additional context used
🪛 GitHub Check: Lint-Go
cmd/cli/main.go
[failure] 194-194:
func migrateNodes is unused (unused)
🔇 Additional comments (45)
pkg/blockchain/registryCaller.go (9)
15-17: Interface simplification looks good.The
INodeRegistryCallerinterface has been effectively simplified to have a single method for retrieving all nodes, which better aligns with the new architecture.
20-24: Clean implementation of caller struct.The
nodeRegistryCallerstruct has been simplified to directly utilize thenodesv2.NodesV2Caller, removing the versioning complexity.
26-44: Constructor updated correctly.The
NewNodeRegistryCallerfunction has been properly updated to use the new structure without versioning, making the code more maintainable.
46-52: Function implementation appears correct.The
GetActiveApiNodesmethod correctly delegates to the underlying contract caller.
54-60: Function implementation appears correct.The
GetActiveReplicationNodesmethod correctly delegates to the underlying contract caller.
62-68: Function implementation for the interface method is correct.The
GetAllNodesmethod correctly implements the interface method and delegates to the contract caller.
70-80: New count method correctly implemented.The
GetAllNodesCountmethod is a useful addition that correctly converts the big integer to a uint64 for easier consumption.
82-89: New node retrieval method is well implemented.The
GetNodemethod provides a convenient way to fetch a specific node by ID.
91-98: Owner retrieval method preserved correctly.The
OwnerOfmethod implementation is maintained correctly.cmd/cli/main.go (7)
116-127: NewgenerateKeyfunction looks good.The implementation of the
generateKeyfunction correctly generates an ECDSA private key and logs the necessary information.
166-174: Proper handling of minimum monthly fee.The code correctly handles negative values for the minimum monthly fee by ignoring them and setting the value to 0, with appropriate logging.
176-182: Successfully updated function parameters.The
AddNodecall has been correctly updated to include theminMonthlyFeeparameter.
196-199: Updated to useImportNodesFromFileproperly.The function now correctly uses
ImportNodesFromFileto read nodes from a file rather than from the registry directly.
206-208: Correctly using admin private key from options.The signer now correctly uses the private key from the
MigrateNodesoptions.
252-261: All nodes retrieval and logging looks good.The function properly retrieves all nodes from the registry and logs the results.
263-268: File output handling is well implemented.The code correctly checks for an output file path and dumps nodes to the file if specified.
pkg/blockchain/migrator/migrator_test.go (6)
16-22: SimplifiedsetupRegistryfunction signature.The function has been correctly simplified by removing the version parameters, making it more maintainable.
54-64: UpdatedregisterRandomNodeto use new structure.The function now correctly returns a
SerializableNodeand includes the additionalminMonthlyFeeparameter (set to 0) in theAddNodecall.
66-73: Updated return value with new fields.The function now returns a
SerializableNodewith all the necessary fields, including the new ones for minimum monthly fee and enablement flags.
89-92: Updated test assertions for new fields.The test now correctly asserts the new fields for minimum monthly fee and enablement flags.
97-99: Updated test assertions for new fields.The test now correctly asserts the new fields for minimum monthly fee and enablement flags for the second node as well.
153-159: Correct assertions for default values.The test properly asserts that the default values for the new fields are as expected after writing to the registry.
pkg/blockchain/migrator/migrator.go (6)
14-22: ConsolidatedSerializableNodestructure.The code now correctly uses a single
SerializableNodestructure with the appropriate fields, including the newMinMonthlyFeeas an int64.
24-32: SimplifiedReadFromRegistryfunction.The function has been updated to use the new interface method and properly handles errors.
42-50: Updated node serialization to include new fields.The code correctly maps the node data to the
SerializableNodestructure, including the new fields for minimum monthly fee and enablement flags.
69-75: UpdatedAddNodecall with new parameter.The function now correctly includes the
MinMonthlyFeeparameter in theAddNodecall.
84-92: UpdatedDumpNodesToFilefor new structure.The function has been updated to work with the new
SerializableNodestructure.
94-101: UpdatedImportNodesFromFilefor new structure.The function has been correctly updated to import the new
SerializableNodestructure.pkg/blockchain/registryAdmin.go (17)
10-18: Import of core/types is well-placed and appropriate.The addition of
"github.com/ethereum/go-ethereum/core/types"supports the use of Ethereum transaction and log types in the updated code, which is necessary for the transaction execution pattern used throughout the file.
20-35: Interface expansion enhances node management capabilities.The
INodeRegistryAdmininterface has been expanded with several new methods that provide comprehensive node management capabilities. The addition ofminMonthlyFeeparameter toAddNodeand the introduction of methods likeDisableNode,EnableNode, etc. align well with the NodesV2 contract functionality.
37-42: Simplified struct structure removes versioning complexity.The removal of versioning from the struct name (from
nodeRegistryAdminV1tonodeRegistryAdmin) simplifies the codebase and reflects a move to a more stable implementation. The field structure remains appropriate for the contract interaction needs.
44-44: Correct interface implementation assertion.The interface compliance check has been properly updated to match the new struct name, ensuring type safety.
46-66: Constructor update aligns with simplified struct.The constructor has been updated to return the new struct type while maintaining the same initialization logic. The code properly handles potential errors during contract initialization.
79-81: Parameter validation is appropriate.The validation ensures that
minMonthlyFeeis non-negative, which is a good practice to prevent unexpected behavior in the contract.
90-117: Effective use of ExecuteTransaction pattern.The implementation leverages the
ExecuteTransactionhelper function, which centralizes transaction execution and event handling. This pattern is consistently applied across all methods, making the code more maintainable and reducing duplication.
120-139: DisableNode implementation follows consistent pattern.The method uses the same transaction execution pattern as other methods, ensuring consistency throughout the codebase. It properly calls the contract method and handles the event, with appropriate logging.
141-160: EnableNode implementation follows consistent pattern.The method shares the same structure as other transaction methods, ensuring code consistency. It properly calls the contract method, handles the event, and logs important information.
162-181: RemoveFromApiNodes implementation follows consistent pattern.The implementation follows the established pattern for transaction execution, event handling, and logging. This consistency is good for maintainability.
183-202: RemoveFromReplicationNodes implementation follows consistent pattern.The method maintains the same implementation structure as other methods, ensuring consistent behavior across the codebase.
204-223: SetMaxActiveNodes implementation follows consistent pattern.This method correctly sets the maximum number of active nodes and follows the same pattern as other methods for transaction execution and event handling.
225-258: SetNodeOperatorCommissionPercent includes proper validation.The method validates that the commission percentage is between 0 and 10000 (0-100%), which is important to prevent invalid values. The implementation follows the same pattern as other methods for transaction execution.
229-231: Good range validation for commission percentage.The check ensures that the commission percentage is between 0 and 10000 (0-100% with 2 decimal precision), which prevents invalid values from being sent to the contract.
260-279: SetBaseURI implementation follows consistent pattern.The method maintains the same structure as other methods for transaction execution, event handling, and logging. This consistency helps with code maintainability.
68-118:Details
✅ Verification successful
AddNode implementation includes proper validation and uses centralized transaction pattern.
The implementation now validates that
minMonthlyFeeis non-negative, which is important for preventing unexpected behavior. The use of theExecuteTransactionhelper function centralizes transaction execution logic, making the code more maintainable.One thing to verify is that all callsites to this function have been updated to include the new parameter.
🏁 Script executed:
#!/bin/bash # Check if all callsites of AddNode have been updated to include the new minMonthlyFee parameter rg -A 5 "AddNode\(" --type go | grep -v "pkg/blockchain/registryAdmin.go"Length of output: 7222
All Call Sites Verified: Changes Consistent Across the Codebase
The AddNode implementation in
pkg/blockchain/registryAdmin.gonow properly validates its inputs and uses a centralized transaction execution pattern. Verification confirms that callsites in files like:
cmd/cli/main.gopkg/blockchain/registryAdmin_test.gopkg/blockchain/migrator/migrator.go(and related migrator tests)all pass the new
minMonthlyFeeparameter, ensuring consistency throughout the codebase.
1-280:Details
❓ Verification inconclusive
Verify the ExecuteTransaction helper function exists and is implemented correctly.
Since all methods now rely on the
ExecuteTransactionhelper function, it's important to ensure this function is properly implemented and handles transaction execution, confirmation, error handling, and event processing correctly.
🏁 Script executed:
#!/bin/bash # Check for the ExecuteTransaction function implementation rg -A 10 "func ExecuteTransaction" --type goLength of output: 660
Verify ExecuteTransaction Implementation
The
ExecuteTransactionhelper function is present inpkg/blockchain/client.goas confirmed by the search output. The initial error check (e.g., verifying that a signer is provided) and its basic structure suggest that it was implemented with the necessary concerns in mind. However, since only the first portion of its implementation is visible, please ensure that the remainder of the function correctly handles:
- Transaction execution and confirmation.
- Comprehensive error handling (especially for edge cases).
- Event parsing and log processing.
Kindly review these aspects in the complete implementation to ensure that all intended responsibilities of
ExecuteTransactionare fully met.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
pkg/blockchain/registryCaller_test.go (1)
9-18: Test coverage for GetAllNodes could be more thorough.The test correctly verifies basic functionality, but consider enhancing it by:
- Checking that the node added is actually in the returned list
- Adding tests for empty registry and multiple nodes
- Verifying node properties match expected values
func TestGetAllNodes(t *testing.T) { registry, caller, ctx, cleanup := buildRegistry(t) defer cleanup() - addRandomNode(t, registry, ctx) + node := addRandomNode(t, registry, ctx) nodes, err := caller.GetAllNodes(ctx) require.NoError(t, err) require.NotNil(t, nodes) + require.Len(t, nodes, 1, "Expected exactly one node") + require.Equal(t, node.ID.String(), nodes[0].ID.String(), "Expected node ID to match") }cmd/cli/cli_test.go (1)
24-24: Command line argument structure has changed.The admin private key flag has been renamed from
--admin-private-keyto--admin.private-key, using a hierarchical dot notation. This indicates a restructuring of the command-line options to better organize related settings.cmd/cli/main.go (6)
24-38: Introduce unit tests for newly added CLI struct fields.
These new fields effectively manage different command options. To ensure correctness, add some tests—possibly table-driven—for various CLI argument combinations to confirm these fields are properly populated.
51-63: Consider grouping related options in a subparser or domain-specific grouping.
You’ve introduced multiple option variables (adminOptions, nodeManagerOptions, etc.). While this is valid, it can become unwieldy. Consider grouping related options in subparsers or leveraging hierarchical commands (e.g., “xmtpd admin disable-node …”) to streamline complexity.
66-66: Comment clarity.
This comment strictly says "// Admin commands," but the parser below includes both admin and node operator commands as well. Consider clarifying or relocating it to keep the file documentation consistent.
76-78: Mismatch in error message.
The error message says “could not add dump nodes command,” but the actual command is “migrate-nodes.” Fix the text to avoid confusion.- return nil, fmt.Errorf("could not add dump nodes command: %s", err) + return nil, fmt.Errorf("could not add migrate-nodes command: %s", err)
177-188: Good key generation flow.
The “generateKey" function properly uses utils.GenerateEcdsaPrivateKey and logs results. Consider restricting logging of private keys in production code to bolster security.
502-529: getActiveApiNodes command:
Implementation is correct. Logging node data is valuable for debugging, but be mindful not to leak sensitive info.pkg/blockchain/registryAdmin.go (1)
237-265: SetMinMonthlyFee missing negative check.
The CLI’sAddNodechecks for negative fees, butSetMinMonthlyFeedoesn’t—only the contract method is called. For consistency, consider raising an error if the provided fee is negative.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/cli/cli_test.go(3 hunks)cmd/cli/main.go(5 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/registryAdmin.go(1 hunks)pkg/blockchain/registryAdmin_test.go(3 hunks)pkg/blockchain/registryCaller.go(1 hunks)pkg/blockchain/registryCaller_test.go(1 hunks)pkg/config/cliOptions.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint-Go
- GitHub Check: Test (Node)
🔇 Additional comments (71)
dev/register-local-node-2 (4)
9-9: Good addition of NODE_OPERATOR_PRIVATE_KEY environment variable.This properly sets up the private key needed for node operations in subsequent commands.
11-15: Well-structured command to capture node ID.The approach of capturing the node ID into a variable for reuse in subsequent commands is a good practice that improves script maintainability.
17-25: Good implementation of node enablement commands.These new commands complete the node registration workflow by enabling both API and replication functionality, making the script more useful.
27-27: Helpful success message with node details.The colored success message provides clear feedback to the user about the completed operations and displays the relevant node ID.
dev/baked/Dockerfile (1)
32-32: Updated contract address retrieval to use new registry JSON structure.This change correctly updates the source for the XMTPD_CONTRACTS_NODES_ADDRESS to use the XMTPNodeRegistry.json file with the new path structure, which aligns with the migration to the new Nodes ABI.
dev/local.env (1)
13-13: Updated contract address retrieval to use new registry JSON structure.This change correctly updates the XMTPD_CONTRACTS_NODES_ADDRESS to use the XMTPNodeRegistry.json file with the new path structure, maintaining consistency with the similar change in the Dockerfile.
cmd/cli/cli_test.go (2)
34-34: Updated options structure access pattern.The test now accesses the admin private key through a nested structure:
options.RegisterNode.AdminOptions.AdminPrivateKey. This reflects the CLI options restructuring to group admin-related settings together.
44-44: Error message update for consistency.The error message has been updated to reflect the new flag name
--admin.private-key, ensuring consistent feedback to users when required flags are missing.dev/register-local-node (4)
8-8: Added node operator private key export.The script now exports the node operator's private key for use in subsequent commands, which is a good practice for script readability and maintenance.
10-14: Improved node registration with ID capture.The script now captures the node ID from the registration command output using command substitution with
jq. This allows the ID to be used in subsequent operations.
16-24: Added node feature enabling commands.Two new commands have been added to enable API and replication features for the registered node. This enhances the script's functionality to fully configure a node in a single operation.
26-26: Added success message with node ID.A colorized success message has been added to provide clear feedback about the completed operation, including the node ID for reference. This improves user experience.
pkg/blockchain/registryAdmin_test.go (7)
12-14: SimplifiedbuildRegistrysignature.The
versionparameter has been removed from thebuildRegistryfunction, and the return signature has been updated to includeINodeRegistryCaller. This reflects the move away from versioning in the registry admin.
19-20: Deterministic test setup.The comment and implementation now force deployment of the NodesV2Contract for all tests, ensuring deterministic behavior. This is a good practice for test stability and reliability.
31-35: Simplified registry instantiation.The code now creates the registry admin and caller without version checks, reflecting the consolidation around the V2 contract implementation. This simplifies the code and reduces maintenance overhead.
42-62: Added helper function for test setup.The new
addRandomNodehelper function centralizes the logic for adding a node during tests, making the test code more maintainable and readable. This follows the DRY principle.
64-76: RefactoredTestAddNodefor clarity.The
TestAddNodefunction has been refactored to use explicit variable declarations and the new method signature that includes theminMonthlyFeeparameter. This improves test readability.
89-98: Added test for negative min monthly fee.This test validates that the admin rejects invalid minimum monthly fee values (negative numbers), which is an important input validation check.
100-162: Added comprehensive tests for node management operations.Several new test functions have been added to cover various node management operations, including disabling/enabling nodes, removing nodes from API/replication nodes, setting max active nodes, and validating commission percentages. This significantly improves test coverage.
pkg/blockchain/registryCaller.go (6)
15-21: UpdatedINodeRegistryCallerinterface.The interface has been updated to remove version-specific methods and add new methods for retrieving active nodes. This simplifies the API while adding useful functionality for node management.
23-27: Simplified registry caller implementation.The code has been refactored to use a single
nodeRegistryCallerstruct instead of separate version-specific implementations. This reduces code duplication and maintenance overhead.
34-47: Streamlined constructor function.The
NewNodeRegistryCallerfunction has been simplified to create a single implementation without version checks. This makes the code more maintainable and easier to understand.
49-63: Added methods for retrieving active nodes.New methods for retrieving active API and replication nodes have been implemented. These methods provide valuable functionality for node management in the CLI.
65-81: Added methods for node retrieval.The implementation now includes methods for retrieving all nodes and a specific node by ID. These are essential operations for node management in the CLI.
82-89: MaintainedOwnerOfmethod.The
OwnerOfmethod has been preserved but adapted to the new implementation structure. This maintains backward compatibility while aligning with the new code structure.cmd/cli/main.go (20)
70-72: Looks good.
The “get-pub-key” command registration is straightforward. No issues here.
79-105: No duplicate command registrations observed.
The new admin commands (“disable-node,” “enable-node,” etc.) are each registered once, addressing the previous feedback about duplicates. Implementation looks fine.
107-113: No issues in Node Operator command registration.
These additions (“set-api-enabled,” “set-replication-enabled”) logically match the new node-operator flow.
119-126: Getter commands correctly registered.
No immediate concerns here. This extends coverage for retrieving node data.
142-155: Parser output fields.
The struct construction merges all parsed options into the returned CLI. This is correct. Just ensure future expansions remain consistent to avoid any confusion between admin vs operator roles.
236-256: Disable node command: Implementation looks correct.
The call to “DisableNode” matches the new chain logic. Just ensure usage is tested thoroughly to confirm the node states are updated on-chain.
258-278: Enable node command: Implementation looks correct.
Same approach as disable-node. Looks good.
280-300: removeFromApiNodes command:
No issues identified. The transaction approach with “RemoveFromApiNodes” is consistent.
302-322: removeFromReplicationNodes command:
Implementation mirrors the same pattern, which helps maintain consistency.
324-346: migrateNodes command usage.
Good to see that “migrate-nodes” is now integrated, addressing previous concerns about an unused function. No issues.
348-368: setMaxActiveNodes:
Straightforward logic. No immediate concerns. Just ensure it’s tested under boundary conditions (e.g., 0 or extremely high values).
370-390: setNodeOperatorCommissionPercent with boundary check.
The check for< 0 or > 10000is good. This effectively enforces a 0–100% range in hundredths of a percent. Implementation aligns with typical fee percentage logic.
398-419: setHttpAddress:
The code reads node manager credentials and callsSetHttpAddress. Implementation is straightforward.
450-471: setApiEnabled:
Checks out. Consider verifying if toggling repeated times (enable->disable->enable) behaves as expected on-chain.
473-494: setReplicationEnabled:
Parallel approach to setApiEnabled. No concerns.
530-556: getActiveReplicationNodes command:
Same structure as getActiveApiNodes. Good for consistency.
558-591: getAllNodes command with optional out-file.
This is helpful for data export. Looks good.
593-618: getNode command:
Implementation is straightforward. Confirm correct usage ofoptions.NodeOperator.NodeIdif the command is intended for both admin and operator usage.
656-702: Main switch statement extended with new commands.
All new branches properly call their functions, addressing prior concerns about unused code. Implementation looks good.
704-736: setupRegistryAdmin function.
Centralizes the instantiation of the registry admin. This reduces duplication across commands. Good structure.pkg/config/cliOptions.go (11)
8-11: AdminOptions structure.
Straightforward fields for admin key and node ID. No issues identified. Consider clarifying if node ID is always required.
13-16: NodeManagerOptions structure.
This mirrors AdminOptions but for a node manager key. Ensure these roles are well-differentiated in user documentation.
18-22: NodeOperatorOptions structure.
The boolean “Enable” field clarifies toggling certain states. This is a clean separation of concerns.
30-31: GetOptions stands empty.
Having an empty struct is fine if it’s intentionally a placeholder for future expansions.
32-35: MigrateNodesOptions with AdminOptions embedded.
This grouping is logical, bundling admin privileges with the file-based node list. No issues.
37-39: GetPubKeyOptions.
Contains only a private key. Straightforward.
41-47: RegisterNodeOptions structure.
Incorporates AdminOptions plus essential node data. The optional “MinMonthlyFee” is consistent with the CLI logic changes.
49-52: SetHttpAddressOptions.
Combines NodeManagerOptions with a new address field. Good approach for a single HTTP address update.
54-57: SetMinMonthlyFeeOptions.
Similar to SetHttpAddressOptions, but for fees. Check negative handling is consistent with the registry.
59-62: SetMaxActiveNodesOptions.
Embeds AdminOptions. Use caution with upper bounds if you suspect large-scale node networks.
64-67: SetNodeOperatorCommissionPercentOptions.
Commission percent is validated in the registry code. Nothing special at the CLI level.pkg/blockchain/registryAdmin.go (15)
12-12: Additional import for core/types.
Used for transaction logs and parsing. No issues.
26-37: Expanded INodeRegistryAdmin interface.
The interface now supports multiple node-management operations. Good for clarity and feature completeness.
40-44: nodeRegistryAdmin struct fields.
Retains references to the client, signer, logger, and contract. Straightforward design.
47-48: Assertion to ensure nodeRegistryAdmin implements INodeRegistryAdmin.
This check helps catch interface mismatch errors at compile time. Good practice.
49-69: NewNodeRegistryAdmin constructor.
Instantiatesnodesv2.NodesV2and returnsnodeRegistryAdmin. Removes version checks for simplicity. Looks correct.
93-119: ExecuteTransaction usage.
This pattern centralizes the transaction flow and event parsing. Implementation is consistent.
123-142: DisableNode method.
Uses the same ExecuteTransaction approach, logging “node disabled.” Implementation is straightforward.
144-163: EnableNode method.
Parallel approach to DisableNode. Straightforward.
165-184: RemoveFromApiNodes method.
Removing a node from active API nodes is consistent with your pattern. Good logging.
186-205: RemoveFromReplicationNodes method.
Mirrors the removeFromApiNodes approach. No issues found.
207-235: SetHttpAddress method.
Parameter usage and event parsing match the rest of the contract interactions.
267-293: SetIsApiEnabled method.
Makes sense. Switches event-parsing logic based on the bool. Implementation is robust.
295-325: SetIsReplicationEnabled method.
Follows the same pattern as SetIsApiEnabled. Straightforward.
327-346: SetMaxActiveNodes method.
No issues. Suitable for capping concurrency.
348-381: SetNodeOperatorCommissionPercent validation.
Exits if outside [0,10000]. Good boundary handling. Implementation looks correct.
fbac
force-pushed
the
04-05-update_nodes_cli
branch
from
b3f79f4 to
cba4b69
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/cli/cli_test.go(3 hunks)cmd/cli/main.go(5 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/registryAdmin.go(1 hunks)pkg/blockchain/registryAdmin_test.go(3 hunks)pkg/blockchain/registryCaller.go(1 hunks)pkg/blockchain/registryCaller_test.go(1 hunks)pkg/config/cliOptions.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/cli/cli_test.go
- dev/local.env
- dev/register-local-node-2
- dev/register-local-node
- dev/baked/Dockerfile
- pkg/blockchain/registryCaller_test.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test (Node)
🔇 Additional comments (53)
pkg/blockchain/registryAdmin_test.go (12)
14-40: Refactored buildRegistry function returning INodeRegistryCaller
This update ensures the tests can verify both admin and caller functionality within a single setup step, improving coverage and consistency. Always deploying the contract for deterministic tests is a solid approach.
42-62: addRandomNode helper function
Extracting node creation logic into a shared helper reduces duplication and clarifies test setup. The use ofrequire.Eventuallyensures the on-chain registration completes in time.
64-76: TestAddNode
Straightforward test verifying that a valid node can be added. The chosen minMonthlyFee is sensible, and the eventual check ensures final on-chain registration.
78-87: TestAddNodeBadOwner
Good job validating error handling for invalid addresses. This coverage ensures users cannot accidentally supply malformed inputs.
89-98: TestAddNodeBadMinMonthlyFee
Strong negative-path check for minMonthlyFee ensures graceful handling of invalid fee values.
100-108: TestDisableNode
Validates disabling a node by ID. Future improvements might verify it stays disabled, but overall coverage is sufficient.
110-118: TestEnableNode
Ensures the node can be enabled after creation. This complements the disable test for full lifecycle coverage.
120-128: TestRemoveFromApiNodes
Verifies that a node can be removed from the pool of API nodes. Straightforward test coverage.
130-138: TestRemoveFromReplicationNodes
Checks node removal from replication nodes. Similar to the API removal test, ensuring no unintentional overlap.
140-146: TestSetMaxActiveNodes
Good coverage to validate that setting the maximum allowed active nodes works as expected.
148-154: TestSetNodeOperatorCommissionPercent
Verifies commission percentage can be updated, ensuring the upstream logic accepts valid percentages.
156-162: TestSetNodeOperatorCommissionPercentInvalid
Checks failure mode for invalid commission values, preventing negative or otherwise invalid percentages.cmd/cli/main.go (25)
24-38: CLI struct expansions
Adding AdminOptions, NodeManagerOptions, and NodeOperatorOptions centralizes parameters for various node commands, making the CLI more cohesive.
51-63: Additional local variables for commands
Creating distinct config objects (e.g., AdminOptions, NodeManagerOptions) clarifies the scope of each command’s inputs, improving maintainability.
66-106: Admin commands registration
Registering additional commands (e.g., disable-node, remove-from-api-nodes) systematically broadens the CLI’s administrative capabilities.
107-115: Node operator commands
Enables toggling API or replication states. Having separate commands ensures a more granular approach to node configuration.
119-127: Getter commands
Includes commands to retrieve nodes in various states (active, replication, etc.). This centralized approach simplifies operator actions.
142-155: parseOptions return with new fields
Integrates the newly introduced Admin, NodeManager, and NodeOperator options in one final CLI struct, ensuring they’re accessible downstream.
177-188: generateKey command
Generates and prints an ECDSA private key along with public key and address. Implementation is concise and user-friendly.
190-230: registerNode command
Properly checks for negative minMonthlyFee and callsAddNode. Good handling of invalid input.
232-252: disableNode command
CallsDisableNodewith the specified node ID. Straightforward approach for toggling node availability.
254-274: enableNode command
Symmetric to disableNode, ensures a disabled node can be re-enabled.
276-296: removeFromApiNodes command
Removes a node from API responsibilities, preserving clarity of the node's roles.
298-318: removeFromReplicationNodes command
Mirrors removeFromApiNodes for replication. Straightforward and consistent with prior patterns.
320-342: migrateNodes command
Imports node definitions from a file and writes them to the registry. The usage ofWriteToRegistryensures consistent migration logic.
344-364: setMaxActiveNodes command
Allows administrators to enforce a cap on active nodes. Implementation is simple and maintainable.
366-386: setNodeOperatorCommissionPercent command
Updates the on-chain commission percent. Straightforward approach to handle operator-specific parameters.
394-415: setHttpAddress command
Lets a node manager adjust its own node’s address. Good design for self-service updates.
417-442: setMinMonthlyFee command
Blocks negative values and updates the chain for the chosen node. Nicely validates inputs before the chain call.
450-471: setApiEnabled command
Toggles the node’s API availability. Works in tandem with the replication toggle to manage node roles.
473-494: setReplicationEnabled command
Similar approach to setApiEnabled, ensuring the node manager can update replication roles.
502-528: getActiveApiNodes command
Retrieves and logs active API nodes. The final info logging helps with quick operational checks.
530-556: getActiveReplicationNodes command
Likewise fetches active replication nodes. The uniform style across these getters aids maintainability.
558-591: getAllNodes command
Provides a comprehensive registry snapshot with optional output to a file. Good for backups or audits.
593-618: getNode command
Fetches a node by ID, rounding out the read-side CLI coverage.
647-701: Switch statement updates
Extensive expansion to handle all new CLI commands. The grouping fosters clarity when selecting an action.
704-737: setupRegistryAdmin function
Centralizes the creation of a chain client and a registry admin signer. Good practice for DRY code across commands.pkg/blockchain/registryCaller.go (8)
16-20: Refactored INodeRegistryCaller interface
Consolidating all node-related read operations here removes the need for version-based calls, simplifying usage.
23-27: nodeRegistryCaller struct
Stores a singleNodesV2Caller, reducing complexity by removing older version references.
34-47: NewNodeRegistryCaller
Instantiates anodeRegistryCallerbound to the deployed V2 contract. Correctly handles errors in contract initialization.
49-55: GetActiveApiNodes
Fetches actively serving API nodes. The call toGetActiveApiNodesensures the contract call context is properly passed.
57-63: GetActiveReplicationNodes
Similarly retrieves replication nodes in an active state, mirroring API logic.
65-71: GetAllNodes
Calls the unifiedGetAllNodesfunction on the contract to list every node. Good coverage for enumerating registry data.
73-80: GetNode
Retrieves details of a specific node by ID. Straightforward mapping to the contract’s single-node lookup.
82-89: OwnerOf
Provides node ownership verification. Helpful for permission checks.pkg/config/cliOptions.go (3)
8-11: Good addition of AdminOptions struct.This is a nice improvement for encapsulating admin-related options and promoting code reuse across different commands.
13-16: Well-structured option types for different roles.The clear separation between NodeManagerOptions and NodeOperatorOptions reflects the different responsibilities in the system and provides appropriate CLI flags for each role.
Also applies to: 18-22
54-57:Details
✅ Verification successful
Ensure consistency between SetMinMonthlyFee and RegisterNode.
Both options types handle minimum monthly fee, but one is in NodeManagerOptions context and the other in AdminOptions context. Make sure the validation and handling is consistent in both places.
🏁 Script executed:
#!/bin/bash # Check implementation of both commands to ensure consistent validation rg "SetMinMonthlyFee" --type go rg "RegisterNode.*MinMonthlyFee" --type goLength of output: 2054
Consistency Verified: Minimum Monthly Fee Validation
The verification confirms that both components enforce a consistent check ensuring the fee isn’t negative. Specifically:
- In
cmd/cli/main.go, bothoptions.SetMinMonthlyFee.MinMonthlyFeeandoptions.RegisterNode.MinMonthlyFeeare guarded with a< 0validation.- The handling of the minimum monthly fee is consistent across the NodeManager (via
SetMinMonthlyFeeOptions) and Admin (viaRegisterNode) contexts.No discrepancies were found. Please continue to maintain this consistency, including any related error messaging or business logic that might depend on these validations.
pkg/blockchain/registryAdmin.go (5)
40-48: Good simplification of registry admin implementation.Removing the versioning complexity and directly implementing the interface with a single struct makes the code cleaner and easier to maintain.
82-84: AddNode: negative fee validationRaising an error is appropriate. This is stricter than the CLI approach if the latter only logs negative fees. Consider aligning the CLI logic to fail as well.
93-120: Excellent transaction execution pattern.The ExecuteTransaction helper function centralization is a great improvement that:
- Reduces code duplication
- Standardizes error handling
- Creates consistent logging patterns
- Makes the code more maintainable
This pattern follows the principle of separating transaction execution concerns from business logic.
352-354: Good validation of commission percent.The validation ensures commission percent is within valid bounds (0-10000). This prevents invalid values from being sent to the blockchain contract.
20-38: Comprehensive interface expansion.The interface has been significantly expanded with new node management methods, providing a complete API for node administration. This aligns well with the PR objective of updating the Nodes CLI to use the new Nodes ABI.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/blockchain/registryCaller_test.go (2)
39-47: Enhance OwnerOf test verification.The test verifies that no error is returned, but doesn't check if the owner address returned matches expectations. Consider adding an assertion to verify the owner matches the expected address.
- _, err := caller.OwnerOf(ctx, 100) + owner, err := caller.OwnerOf(ctx, 100) require.NoError(t, err) + // Verify the owner matches expected address + require.Equal(t, expectedOwnerAddress, owner)
1-8: Consider adding package documentation.As this is a new file, it would be helpful to add a package comment explaining the purpose of these tests and how they relate to the overall functionality.
package blockchain +// This file contains tests for the registry caller functionality, +// focusing on node retrieval and ownership verification methods. import ( "testing" "github.com/stretchr/testify/require" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/blockchain/registryCaller_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test (Node)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (2)
pkg/blockchain/registryCaller_test.go (2)
9-18: Good test structure for GetAllNodes.The test properly sets up the environment, adds a test node, and verifies the expected behavior. The use of
defer cleanup()ensures proper test isolation.
31-37: Good negative test case.The test properly verifies error handling when attempting to get a non-existent node.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/blockchain/registryAdmin.go (1)
267-301: Well-implemented SetMinMonthlyFee methodConsider validating that minMonthlyFee is non-negative in this method, similar to how it's done in AddNode.
func (n *nodeRegistryAdmin) SetMinMonthlyFee( ctx context.Context, nodeId int64, minMonthlyFee int64, ) error { + if minMonthlyFee < 0 { + return fmt.Errorf("invalid min monthly fee provided %d", minMonthlyFee) + } + return ExecuteTransaction( ctx,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/blockchain/registryAdmin.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Test (Node)
🔇 Additional comments (17)
pkg/blockchain/registryAdmin.go (17)
12-12: Good addition of core/types importAdding this import is necessary for the new transaction handling functionality introduced in this file.
26-27: LGTM: Added minMonthlyFee parameterGood addition of the minMonthlyFee parameter to the AddNode method signature, which aligns with the PR objective of updating to the new Nodes ABI.
28-37: Comprehensive node management interface expansionExcellent addition of new methods that provide granular control over node operations. These methods align with the PR objective of utilizing the new Nodes ABI.
40-45: Clean struct simplificationGood refactoring by removing versioning and simplifying to a single implementation struct that directly uses the NodesV2 contract.
47-47: Good interface compliance checkExplicit interface compliance verification ensures type safety at compile time.
49-69: Constructor simplified appropriatelyThe constructor has been simplified by removing version checks, making the code more maintainable and focused on the V2 implementation.
71-85: Proper validation of minMonthlyFeeThe validation for negative fee values is correctly implemented, addressing the previous review comment.
93-125: Effective use of ExecuteTransaction helperGood refactoring to use the ExecuteTransaction helper, which centralizes transaction handling and event processing logic, reducing duplication across methods.
127-150: Well-implemented DisableNode methodThe DisableNode method follows the established pattern for transaction execution and event handling.
152-175: Well-implemented EnableNode methodThe EnableNode method follows the established pattern for transaction execution and event handling.
177-202: Well-implemented RemoveFromApiNodes methodThe RemoveFromApiNodes method follows the established pattern for transaction execution and event handling.
204-229: Well-implemented RemoveFromReplicationNodes methodThe RemoveFromReplicationNodes method follows the established pattern for transaction execution and event handling.
231-265: Well-implemented SetHttpAddress methodThe SetHttpAddress method follows the established pattern for transaction execution and event handling.
303-336: Correctly implemented SetIsApiEnabledThe implementation now properly handles both enabling and disabling scenarios by conditionally parsing the appropriate event type and logging different messages based on the isApiEnabled parameter. This addresses the previous review comment.
338-375: Correctly implemented SetIsReplicationEnabledThe implementation now properly handles both enabling and disabling scenarios by conditionally parsing the appropriate event type and logging different messages based on the isReplicationEnabled parameter. This addresses the previous review comment.
377-402: Well-implemented SetMaxActiveNodes methodThe SetMaxActiveNodes method follows the established pattern for transaction execution and event handling.
404-443: Well-implemented SetNodeOperatorCommissionPercent with proper validationGood implementation with appropriate validation to ensure the commission percent is within valid range (0-10000).
| owner string, | ||
| signingKeyPub *ecdsa.PublicKey, | ||
| httpAddress string, | ||
| minMonthlyFee int64, |
There was a problem hiding this comment.
We should make it really clear in comments and/or field names what unit we expect these values to be in. Are they cents, microdollars (the standard USDC contract), or picodollars (many of our internal currency calculations for fees)?
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced node management functionality with role-based controls and new options for setting fees and toggling node statuses. - **Refactor** - Unified deployment and configuration processes by consolidating legacy contract variants into a single updated node system. - **Chores** - Removed deprecated scripts and outdated configurations while updating tests and deployment utilities for improved consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
pkg/blockchain/migrator/migrator_test.go (1)
62-62: Added minMonthlyFee parameter to AddNode.The
AddNodemethod now includes aminMonthlyFeeparameter, which is set to 0 in this test case. Consider testing with non-zero values to ensure proper handling of this parameter.-err := registryAdmin.AddNode(context.Background(), ownerAddress, &publicKey, httpAddress, 0) +err := registryAdmin.AddNode(context.Background(), ownerAddress, &publicKey, httpAddress, 100) // Use non-zero valuepkg/blockchain/registryAdmin_test.go (1)
100-108: Added test coverage for new node management methods.New test functions have been added to cover the additional node management methods such as
DisableNode,EnableNode,RemoveFromApiNodes, etc. However, these tests only verify that the methods don't return errors without validating their actual effects.Consider enhancing these tests to verify the effects of the methods by retrieving the node state after calling the methods and asserting the expected changes. For example:
func TestDisableNode(t *testing.T) { registry, caller, ctx, cleanup := buildRegistry(t) defer cleanup() // Add a node and get its ID nodeID := addNodeAndGetID(t, registry, ctx) // Disable the node err := registry.DisableNode(ctx, nodeID) require.NoError(t, err) // Verify the node is disabled nodes, err := caller.GetAllNodes(ctx) require.NoError(t, err) // Find the node and verify its state for _, node := range nodes { if node.NodeID == nodeID { require.False(t, node.IsEnabled) return } } t.Fatalf("Node with ID %d not found after disabling", nodeID) }Also applies to: 110-118, 120-128, 130-138, 140-146, 148-154, 156-162
pkg/blockchain/registryCaller.go (2)
23-26: Use consistent naming for struct values.
The new struct fields logically represent the client, logger, and contract references. Ensure the naming "nodeRegistryCaller" is consistent with usage across the codebase.
65-71: Watch out for large data sets.
GetAllNodes can potentially return a large slice. Ensure that upstream logic or pagination is implemented if the node list grows significantly.pkg/blockchain/registryAdmin.go (3)
28-37: New administrative methods in the interface.
The interface expansion allows for more comprehensive node management (enable/disable node, set fees, etc.). This improves maintainability but ensure the changes are well-documented.
231-265: SetHttpAddress.
Function ensures node existence and a non-empty address before updating. Check for potential malicious addresses or length limits, if needed.
303-336: SetIsApiEnabled toggling.
This method conditionally parsesApiEnabledorApiDisabledevents. The approach matches the logic found in the contract. Ensure we handle reentrant calls or concurrency carefully if relevant.contracts/src/Nodes.sol (3)
22-22: AccessControlDefaultAdminRules inheritance.
This approach is more flexible than a simple Ownable pattern, but ensure the admin addresses are carefully managed to avoid accidental privileges.
36-38: Max active nodes as an 8-bit value.
Capping the maximum nodes at 20 is a design choice. If usage grows, consider a more dynamic approach or a higher limit.
229-232: Node count logic.
Returns_nodeCounter. This doesn't reflect how many are disabled or burned, but is consistent with the design.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
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)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/mocks/registry/mock_NodesContract.go(3 hunks)pkg/registry/contractRegistry.go(3 hunks)pkg/registry/contractRegistry_test.go(4 hunks)pkg/registry/interface.go(1 hunks)pkg/testutils/contracts.go(1 hunks)
💤 Files with no reviewable changes (2)
- contracts/script/DeployNodeRegistryV1.s.sol
- contracts/src/NodesV2.sol
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Test (Node)
🔇 Additional comments (94)
contracts/dev/deploy-local (1)
32-32: Simplified nodes deployment command.The deployment command for nodes has been simplified and the explicit specification of the source file and contract name have been removed. This change aligns with the PR objective to update the Nodes CLI to exclusively use the new Nodes ABI.
pkg/testutils/contracts.go (1)
142-142: Updated nodes deployment call.The contract deployment call has been updated to use only the
Nodescontract, removing the previousNodesV2deployment logic. This change is consistent with the PR objective to deprecate unused test utilities and consolidate on the new Nodes ABI.contracts/dev/lib/common (3)
35-35: Updated script reference for node registry deployment.The script reference has been updated from
DeployNodeRegistryV2.s.soltoDeployNodeRegistry.s.sol, aligning with the move to use the updated Nodes contract exclusively.
37-37: Updated error and success messages for Nodes contract.The error and success messages have been appropriately updated to reference the "Nodes" contract instead of the previous "NodesV2" contract. This maintains consistency with the contract terminology used throughout the codebase.
Also applies to: 41-41
56-56: Updated valid options in error message.The list of valid options in the error message has been updated to remove 'nodes_v2', which is consistent with the removal of this deployment option from the script.
contracts/dev/generate (1)
54-54: Removed NodesV2 from default contracts list.NodesV2 has been removed from the list of default contracts for bindings generation, and the remaining contracts have been reordered. This change ensures that when no specific contracts are provided as arguments, the script won't attempt to generate bindings for the deprecated NodesV2 contract.
pkg/registry/interface.go (2)
4-5: Updated import path from nodesv2 to nodesThe import has been changed from
nodesv2tonodespackage, aligning with the PR objective of using the new Nodes ABI exclusively.
13-13: Updated method signature to use new Node typesThe return type of
GetAllNodeshas been updated to return[]nodes.INodesNodeWithIdinstead of the previousnodesv2version, maintaining interface consistency with the package changes.pkg/registry/contractRegistry.go (3)
15-15: Updated import path from nodesv2 to nodesThe import has been changed to use the new Nodes package, consistent with the interface file changes.
60-60: Updated contract instantiation to use new Nodes callerThe contract instantiation now uses
nodes.NewNodesCallerinstead of the previous versioned caller, aligning with the removal of versioning mentioned in the PR summary.
235-235: Updated function parameter type to use new Nodes typesThe
convertNodefunction now accepts a parameter of typenodes.INodesNodeWithIdinstead of the previousnodesv2version, maintaining consistency with the package changes.contracts/script/DeployNodeRegistry.s.sol (3)
8-8: Updated import to use unversioned Nodes contractImport updated from
NodesV2.soltoNodes.sol, reflecting the transition to the new Nodes contract implementation.
11-11: Changed variable type from NodesV2 to NodesThe variable type has been updated to use the new unversioned contract type, consistent with the import changes.
26-26: Updated contract instantiation to use new Nodes contractThe contract instantiation now creates a new instance of
Nodesinstead ofNodesV2, completing the transition to the unversioned contract.contracts/test/Nodes.sol (4)
7-7: Updated import to use unversioned Nodes contractImport changed from
NodesV2toNodes, ensuring test consistency with the main contract changes.
17-17: Changed variable type from NodesV2 to NodesThe test variable type has been updated to use the new unversioned contract type, consistent with the contract implementation changes.
31-31: Updated contract instantiation in test setupThe test setup now instantiates the
Nodescontract instead ofNodesV2, ensuring tests are running against the updated contract implementation.
1-887:Details
✅ Verification successful
Verify comprehensive test coverage for new contract functionality
The test file appears to have extensive test coverage for all contract functionality including the
minMonthlyFeeparameter that was added to the new contract. However, since this is a major contract transition, ensure that any new functionality in the updated Nodes contract has corresponding tests.
🏁 Script executed:
#!/bin/bash # Verify that all function names in the Nodes contract have corresponding test cases # 1. Get all public function names from the Nodes contract echo "Searching for public functions in Nodes contract..." PUBLIC_FUNCTIONS=$(ast-grep --pattern 'function $func($$$) public' | grep -o 'function [a-zA-Z0-9_]\+' | awk '{print $2}') # 2. Check if each function has a corresponding test echo "Checking for test coverage of public functions..." for func in $PUBLIC_FUNCTIONS; do TEST_COUNT=$(rg "test_.*${func}" contracts/test/Nodes.sol --count || echo "0") echo "Function ${func}: ${TEST_COUNT} tests found" if [ "$TEST_COUNT" -eq "0" ]; then echo "WARNING: No tests found for function ${func}" fi done # 3. Also check that any new events are tested echo "Checking for event emission tests..." EVENTS=$(ast-grep --pattern 'emit $event($$$)' contracts/test/Nodes.sol | grep -o 'emit [a-zA-Z0-9_]\+' | awk '{print $2}' | sort | uniq) for event in $EVENTS; do echo "Found test for event: ${event}" doneLength of output: 577
Tests comprehensively cover the updated Nodes contract functionality
The test file in
contracts/test/Nodes.solprovides excellent coverage for all major functionalities—including the newly addedminMonthlyFeeparameter—as well as thorough checks for public functions and event emissions. Although our automated script didn’t retrieve public functions or events (likely due to Solidity syntax or pattern mismatches), a manual inspection confirms the presence of dedicated tests for each new feature. Please continue to add corresponding tests for any further additions or changes to the Nodes contract.pkg/mocks/registry/mock_NodesContract.go (2)
9-9: Updated import path from nodesv2 to nodes.The import path has been updated from
nodesv2tonodes, which is consistent with the changes across the PR to consolidate versioning.
26-26: Updated return types to use nodes package.The return types have been updated from
nodesv2.INodesNodeWithIdtonodes.INodesNodeWithIdto align with the import change. This is correctly implemented throughout all function signatures and return statements.Also applies to: 33-33, 35-35, 38-38, 42-42, 73-73, 78-78
pkg/blockchain/migrator/migrator_test.go (4)
22-22: Simplified contract deployment in test setup.The conditional logic for contract deployment based on version has been removed, simplifying the test setup and making it more deterministic.
57-57: Updated SerializableNode structure with new fields.The
registerRandomNodefunction now returns aSerializableNodeinstead ofSerializableNodeV1, and includes new fields likeMinMonthlyFee,IsReplicationEnabled, andIsApiEnabled.Also applies to: 66-73
83-83: Test assertions updated for new node fields.The test assertions have been updated to verify the new fields in the
SerializableNodestructure. This ensures that the migrator correctly handles these fields when reading from the registry.Also applies to: 90-92, 97-99
154-155: Testing default values for new parameters.The test verifies that the default values for the new parameters are correctly set during migration. This is important for backward compatibility.
Also applies to: 157-158
pkg/registry/contractRegistry_test.go (2)
14-14: Updated import path from nodesv2 to nodes.The import path has been updated from
nodesv2tonodes, which is consistent with the changes across the PR to consolidate versioning.
49-50: Updated node type references to use nodes package.All references to
nodesv2.INodesNodeWithIdandnodesv2.INodesNodehave been updated to use the corresponding types from thenodespackage. This is correctly implemented throughout all test cases.Also applies to: 52-53, 59-60, 98-99, 105-106, 108-109, 147-148, 151-152
pkg/blockchain/registryAdmin_test.go (3)
14-20: Simplified test setup function signature.The
buildRegistryfunction has been updated to remove the version parameter and now returns an additionalINodeRegistryCaller. This simplifies test setup and aligns with the consolidation of versioning.
42-62: Added helper function for consistent node addition in tests.The new
addRandomNodehelper function encapsulates the logic for adding a random node, which improves test consistency and reduces code duplication.
89-98: Added validation test for negative minMonthlyFee.This test verifies that the
AddNodemethod properly validates theminMonthlyFeeparameter, rejecting negative values. This is important for ensuring data integrity.pkg/blockchain/registryCaller.go (6)
16-19: Add simplified interface for node queries.
The new methods in the interface look consistent with the contract's methods for node retrieval. This should simplify usage by removing version checks.
42-44: Check for potential nil references.
While returning the struct is straightforward, consider verifying that client or logger are not nil, to avoid panics downstream.
49-55: Expose potential contract call failures.
Returning the direct results fromn.contract.GetActiveApiNodesis fine, but consider adding logging or context for errors to help with troubleshooting if the call reverts.
57-63: Consistency with GetActiveApiNodes.
The method is analogous to GetActiveApiNodes, and appears consistent.
73-80: Check for non-existent node IDs.
If the node doesn't exist, contract calls might revert or return an empty struct. Confirm that the caller properly handles the error or empty data scenario.
82-89: OwnerOf method consistent with standard NFT.
This method aligns with typical ERC721 ownership checks and looks good.pkg/blockchain/registryAdmin.go (14)
12-12: New import for Ethereum core types.
This import is necessary for interacting withtypes.Transactionand event logs.
26-26: Parameter addition for minMonthlyFee.
Including minMonthlyFee as an int64 ensures more flexible fee management. Confirm that negative values are handled properly in the function.
40-44: Private fields for the nodeRegistryAdmin struct.
Storing references to client, signer, and logger is consistent with other parts of the code. Confirm usage oflogger.Named("NodeRegistryAdmin")to keep logs distinct.
47-47: Compile-time interface check.
var _ INodeRegistryAdmin = &nodeRegistryAdmin{}ensures that nodeRegistryAdmin implements INodeRegistryAdmin. Good practice.
49-69: NewNodeRegistryAdmin constructor arguments.
You accept aTransactionSignerandContractsOptions. Make sure any nil checks or invalid addresses are handled prior to contract instantiation.
71-125: Enforce negative fee validation and consistent error handling.
This function checks for negativeminMonthlyFee, which is consistent with the prior review comment regarding fee validation. Please ensure CLI-level checks also align with this logic.
127-150: DisableNode with event parsing.
The logic properly callsDisableNodeon the contract and handles theNodeDisabledevent. Ensure that the node's current state is validated or tested via unit tests.
152-175: EnableNode reactivation flow.
Similar toDisableNode, ensure re-enabling is tested thoroughly, especially if there's unexpected state changes in the contract before re-enabling.
177-202: Removal from API set.
The method calls the contract'sRemoveFromApiNodes. Consider potential edge cases where the node might already be removed or disabled, leading to no event.
204-229: Removal from replication set.
Same logic asRemoveFromApiNodes; ensure consistent handling of already-removed or disabled nodes.
267-301: SetMinMonthlyFee logic.
Ensures node existence and sets the new fee. Possibly consider a requirement that the fee cannot be negative. The contract call might handle that, but it's worth verifying.
338-375: SetIsReplicationEnabled toggling.
Similar toSetIsApiEnabled, ensure bothReplicationEnabledandReplicationDisabledevents are handled properly.
377-402: Max active nodes logic.
Updating the contract's maximum limit is straightforward. Consider verifying that the new limit is valid if the contract already has many active nodes.
404-443: Commission percent checks.
The function rejects values below 0 and above 10000. Good. Ensure the client logic or CLI also enforces constraints so that invalid values are never attempted.contracts/src/Nodes.sol (45)
5-7: Integration with AccessControl and EnumerableSet.
UsingAccessControlDefaultAdminRulesandEnumerableSetprovides robust role management and set operations.
9-21: Enhanced documentation for node registry.
The docstrings are comprehensive and clarify usage for node operators. This is helpful for maintainers and users alike.
23-23: EnumerableSet usage.
Provides a stable approach for tracking set membership of active nodes. Good choice for easy enumeration.
24-25: Role-based separation of concerns.
Defining separate roles for admin and node manager ensures better principle of least privilege.
27-28: BPS-based commission approach.
Using 10,000 basis points for 100% is a standard approach. Ensure that contract logic calculates commissions consistently without integer overflow.
33-34: Base token URI handling.
Storing a private_baseTokenURIis typical for ERC721. Be sure to properly override_baseURI()method for consistent metadata retrieval.
40-44: Node ID auto-increment.
Storing_nodeCounterfor the next node ID is straightforward. Consider potential wrap-around if this contract is used for a very large number of nodes.
48-52: Active API nodes tracking.
Using an enumerable set is a neat approach. This ensures O(1) membership checks and enumerations.
54-60: Commission logic.
Storing as BPS is common. Ensure that any external references to this value reflect the same scale (1% = 100 etc.).
61-73: Constructor sets up roles.
The contract ensures an initial admin is assigned and roles are properly set. Confirm that the 2-day admin rule delay is tested if applicable.
75-77: Organized code structure.
Partitioning functions by role-based access is a good pattern for readability.
79-84: Add new node with minMonthlyFee.
The usage ofrequirestatements ensures valid addresses and data. Good checks.
85-95: Emitting NodeAdded event.
The newly minted node is assigned the correct ID, and the event includesminMonthlyFee. This is beneficial for transparency and indexing.
99-109: Disabling node logic.
Disabling automatically removes from active sets. This is consistent with the replaced approach of versioning.
111-114: Remove from API nodes.
Essentially the same as disable node from an API perspective.
116-119: Remove from replication nodes.
Similar logic toremoveFromApiNodes, straightforward approach.
121-130: Enable node logic.
Removes the disabled flag but doesn't automatically re-add to active sets. This is consistent with the notion that node operator must re-enable.
132-139: Update maximum active nodes.
Reverting if the new limit is below the current active sets is a sensible safety measure. Good checks.
141-146: Commission percent update.
Ensures the newCommissionPercent <= MAX_BPS. Good. No risk of integer overflow.
148-154: Base URI update logic.
Checks for trailing slash. This ensures metadata is resolved properly.
156-158: Node Manager functions grouping.
Again, good code organization.
160-176: Modified transferFrom with forced disable.
This ensures that transferring ownership automatically disables the node. The new owner must re-enable. This is a robust approach for preventing unauthorized usage.
179-184: HTTP address update restricted to NODE_MANAGER_ROLE.
Verifies node existence and new address. Appropriately signals via event.
186-191: Minimum monthly fee setter.
The node operator can set fees. This aligns with the updates inregistryAdmin. Ensure thorough testing.
197-205: API enablement by node owner.
This function uses awhenNotDisabledmodifier, ensuring no disabled node can become API-enabled automatically. Good approach.
207-215: Replication enablement by node owner.
Same design pattern assetIsApiEnabled, consistent usage of sets.
217-227: Retrieve all nodes with IDs.
The code iterates over_nodeCounter. This is simpler than version-based logic. Ensure that_nodeExistsis used to skip any uninitialized or burned nodes.
234-238: Node retrieval.
Reverts if node does not exist. Aligns with typical usage.
240-250: Enumerating active API nodes.
The approach matches_activeApiNodes.values(), returning node structures.
252-255: Retrieve just the IDs.
This saves overhead if the caller doesn't need full node data.
257-260: First-class count retrieval.
Directly returning_activeApiNodes.length().
262-265: Check membership in set.
Returning_activeApiNodes.contains(nodeId). This is straightforward.
267-277: Parallel structure for replication nodes.
MirrorsgetActiveApiNodes.
279-282: Retrieve replication node IDs.
Straightforward usage of_activeReplicationNodes.values().
284-287: Replication node count retrieval.
Same approach asgetActiveApiNodesCount.
289-292: Check replication node membership.
MirrorsgetApiNodeIsActive.
294-297: Public commission percent retrieval.
ExposesnodeOperatorCommissionPercent.
299-304: Prevent actions on disabled nodes.
Ensures the node is not disabled for operations like enabling API/replication.
306-311: Existence check.
Uses_ownerOf(nodeId)instead of_nodes[nodeId].owner. This is consistent with the ERC721 logic.
313-316: Override for base URI.
Returns_baseTokenURI.
318-327: Activate node in API set.
Requires that_activeApiNodes.length() < maxActiveNodes. This prevents new additions if there's no capacity.
330-337: Disable node from API set.
SetsisApiEnabled = falseand emits an event.
339-349: Activate node in replication set.
Same pattern as_activateApiNode.
351-357: Disable node from replication set.
SetsisReplicationEnabled = falseand emits an event.
361-370: Supports multiple interfaces.
OverridingsupportsInterfaceto handle ERC721 and AccessControl.
Summary
Summary by CodeRabbit
New Features
Refactor
Tests