XMTP Node Registry enhancements #572
Conversation
|
Warning Rate limit exceeded@fbac has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes introduce a new deployment option for a NodesV2 contract across deployment scripts, smart contract implementations, interfaces, tests, and associated test utilities. The NodesV2 contract now manages nodes with separate mappings for API and replication functionalities, adds new enabling/disabling functions, and renames several functions for clarity. Additionally, the INodes interface is restructured by splitting out error and event definitions, and test cases as well as proxy address handling are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant D as Deployment Script
participant F as Forge Script
participant BC as Blockchain
U ->> D: Execute "forge_deploy_script nodes_v2"
D ->> D: Parse command and select NodesV2 deployment
D ->> F: Invoke DeployNodeRegistryV2 with RPC URL
F ->> BC: Deploy NodesV2 contract
BC -->> F: Return deployment details
F -->> D: Provide success/error response
D -->> U: Output deployment result
sequenceDiagram
participant U as User
participant C as NodesV2 Contract
U ->> C: Call transferFrom() for node transfer
C ->> C: Disable API and Replication node functionalities
C ->> C: Complete ownership transfer
C -->> U: Confirm transfer with node deactivation
Possibly Related PRs
Suggested Reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/testutils/config.go (1)
87-88: Consider maintaining naming consistency.The name "XMTPNodeRegistry" uses a different naming pattern compared to other proxy addresses like "groupMessagesProxy" and "identityUpdatesProxy". This inconsistency could lead to confusion or maintenance issues.
Consider using a consistent naming convention across all proxy addresses:
- case strings.Contains(fileName, "XMTPNodeRegistry.json"): - return fastjson.GetString(data, "addresses", "XMTPNodeRegistry") + case strings.Contains(fileName, "NodesV2.json"): + return fastjson.GetString(data, "addresses", "nodesV2Proxy")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
contracts/dev/deploy-local(1 hunks)contracts/dev/lib/common(1 hunks)contracts/src/NodesV2.sol(6 hunks)contracts/src/interfaces/INodes.sol(7 hunks)contracts/test/Nodes.sol(15 hunks)pkg/testutils/config.go(1 hunks)pkg/testutils/contracts.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Test (Node)
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
🔇 Additional comments (39)
contracts/dev/deploy-local (1)
33-33: LGTM!The addition of
forge_deploy_script nodes_v2correctly enables the deployment of the new NodesV2 contract, complementing the existing deployment options.pkg/testutils/contracts.go (3)
16-16: LGTM!The import for the nodesv2 package is correctly added.
27-27: LGTM!The constant
NODES_V2_CONTRACT_NAMEis appropriately defined and follows the existing naming pattern.
143-145: LGTM!The
DeployNodesV2Contracthelper function correctly follows the pattern of existing deployment functions.contracts/dev/lib/common (2)
40-49: LGTM!The implementation for the nodes_v2 case is well-structured, following the pattern established by other deployment scripts. Error handling is properly implemented, ensuring that deployment failures are caught and reported.
40-49:Details
❌ Incorrect review comment
Verify file naming consistency.
The NodesV2 contract deployment details are written to "XMTPNodeRegistry.json", which doesn't follow the naming pattern of other contracts (where the file is typically named after the contract). This inconsistency could lead to confusion.
🏁 Script executed:
#!/bin/bash # Check why the output JSON file is named "XMTPNodeRegistry.json" instead of "NodesV2.json" grep -r "XMTPNodeRegistry" --include="*.sol" .Length of output: 702
Clarification on Deployment File Naming
The output file naming as "XMTPNodeRegistry.json" is by design. The constant defined in
contracts/script/utils/Environment.sol(XMTP_NODE_REGISTRY_OUTPUT_JSON) and its consistent usage incontracts/script/DeployNodeRegistryV2.s.solindicate that the deployment details for the NodesV2 contract are intentionally written to this file. There is no naming inconsistency in this context.Likely an incorrect or invalid review comment.
contracts/test/Nodes.sol (20)
8-8: Use of separate interfaces is clean and modular.
No issues found.
23-26: Helpful documentation comments.
These reminders clarify the internal helper functions and recommended usage sequence, improving maintainability.
39-55: Event emission andisDisabledcheck align with new interface changes.
Consolidating events underINodesEventsand switching toisDisabledmake the test logic consistent with the updated contract design.
57-79: Correct error usage for invalid node parameters.
LeveragingINodesErrors.InvalidAddress,InvalidSigningKey, andInvalidHttpAddressensures clarity and uniformity across the codebase.
116-140: Disable/enable node test covers critical transitions thoroughly.
This test checks both disabling and re-enabling states effectively. The approach of not automatically re-enabling API/replication flags after un-disabling seems intentional.
142-193: Unauthorized or non-existent node scenarios are properly tested.
Ensuring these revert paths helps maintain robust access control.
199-225: Replication node removal tests follow the same pattern as API node removal.
Maintains consistency in error handling, revert messages, and coverage.
365-419:setHttpAddresstests ensure correct revert handling and required privileges.
Good coverage on the “node does not exist,” “invalid address,” and unauthorized calls.
422-466:setIsApiEnabledchecks properly handle node ownership, node existence, and disabled states.
These tests confirm each revert path is triggered correctly.
469-513:setIsReplicationEnabledtests mirror API enabling checks.
Consistent naming and error usage across the two enabling features improves maintainability.
516-562:setMinMonthlyFeetests check new error codes and role-based restrictions.
Validates that only authorized roles can update the fee.
565-600:setMaxActiveNodeslogic is properly tested, especially for below-current-count scenarios.
This ensures the contract can’t silently reduce capacity below active usage.
603-632:setNodeOperatorCommissionPercentcovers happy path and invalid inputs.
Ensures robust checks on max basis points.
642-642: Event-driven base URI updates.
The single event is consistent with the restructured events approach.
662-669:InvalidURIrevert checks for empty or missing trailing slash.
Appropriately restricts base URL format.
708-708: Consistent alignment onisDisabledfield.
Usingvm.assertEqensures the updated approach works as expected.
713-713: Reverting on non-existent node access.
Properly guards against invalid node IDs.
717-805: Active nodes queries are comprehensive.
Tests thoroughly cover API and replication sets, enabling and disabling flows, and verifying correctness of the results.
839-870: Internal helpers_addNode,_enableNode, and_enableNodes.
These are well-structured, DRY utilities that streamline repeated setup steps.
878-878: DefaultisDisabledset to false aligns with node enable/disable logic.
This initial value avoids unintentional “disabled” states.contracts/src/interfaces/INodes.sol (3)
7-45: Introduction of a dedicatedINodesErrorsinterface.
Having distinct error definitions simplifies error handling and fosters clarity across the codebase.
47-120:INodesEventsinterface neatly segregates events from core logic.
This separation reduces clutter and makes events easier to discover and maintain.
122-281: RefactoredINodesinterface with new fields and function signatures.
AddingisDisabledand splitting functionality (API vs. replication) clarifies node states. The extended getter methods provide precise views into node activity.contracts/src/NodesV2.sol (10)
69-72: Granting bothADMIN_ROLEandNODE_MANAGER_ROLEto_initialAdmin.
Ensures consistent roles for immediate administration.Would you like to confirm no other addresses require these roles on deployment?
98-109:disableNodeproperly setsisDisabledand removes the node from active sets.
This is essential to ensure an administratively disabled node doesn’t remain in API or replication sets.
131-139:setMaxActiveNodeschecks active sets to prevent accidental reduction below the current count.
Helps maintain system stability by disallowing contradictory configurations.
160-176: OverridingtransferFromto deactivate the node prior to ownership change.
Effectively mitigates the risk of leftover active states when ownership changes.
178-184:setHttpAddressrestricted toNODE_MANAGER_ROLE.
Maintains administrative clarity; using a specific role (instead of admin) for node management is a good practice.
186-191:setMinMonthlyFeewriter access restricted toNODE_MANAGER_ROLE.
Aligns with the overarching design of limiting node configuration privileges.
198-215:whenNotDisabledmodifier ensures only active nodes can enable API/replication.
Prevents accidentally enabling features on disabled nodes and enforces consistent state transitions.
218-297: Comprehensive getters for API and replication statuses.
Implementation withEnumerableSetprovides efficient membership checks. The result arrays are built in a loop, which is straightforward for a moderate number of nodes.
299-304:whenNotDisabledensures node existence and active state.
Enhances contract safety by gatekeeping restricted functions.
318-369: Private helper methods_activateApiNode,_disableApiNode,_activateReplicationNode,_disableReplicationNode.
Encapsulating these calls reduces duplication and helps keep logic consistent for each enable/disable path.
#550 was closed in favor of this PR, where the changes will be implemented incrementally to support the migrator tool and further modifications.
Closes #542
Summary by CodeRabbit
New Features
Refactor
Tests
Chores