feat!: enhancements to XMTP Nodes Registry #524
Conversation
WalkthroughThis pull request introduces a new Solidity interface for node management and a new contract, NodesV2, implementing an NFT-based node registry with role-based access control. It also updates the testing suite to cover the new functionalities, adds utility functions for generating test data, adjusts workflow cache configurations, and revises deployment scripts with additional validations and output serialization. Minor formatting changes and metadata updates in the existing contracts accompany these updates. Changes
Sequence Diagram(s)sequenceDiagram
participant E as Environment
participant D as DeployXMTPNodeRegistry
participant N as NodesV2
participant F as FileSystem
D->>E: Retrieve admin address & privateKey
E-->>D: Return admin address & privateKey
D->>N: Deploy NodesV2 (pass admin address)
N-->>D: Return deployed contract address
D->>D: Serialize deployment data into JSON
D->>F: Write JSON deployment output file
D->>D: End deployment broadcast
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: 0
🧹 Nitpick comments (1)
contracts/src/interfaces/INodes.sol (1)
20-23: Events indexing suggestion
Consider indexingnodeIdin the events (e.g.,NodeCreated,NodeActivated,NodeDeactivated) to facilitate easier filtering and off-chain querying.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/src/Nodes.sol(4 hunks)contracts/src/XMTPGovernance.sol(1 hunks)contracts/src/interfaces/INodes.sol(1 hunks)
🔇 Additional comments (15)
contracts/src/XMTPGovernance.sol (5)
1-9: Headers and imports look good
No issues found with the SPDX license identifier, pragma version, or import statements. The high-level comments describing the contract's purpose are clear.
10-19: Use of custom error and contract structure
Defining a customUnauthorizederror is a good gas-saving approach compared to revert strings. The overall contract structure is straightforward, and the documentation comments are concise.
20-22: Constructor clarity
Passing the owner address toOwnable(_owner)is a clear way to set contract ownership. No concerns here.
23-34: Interface validation and assignment
ThesetXMTPNodeRegistryfunction properly checks that the provided contract supports theINodesinterface viaERC165Checker. This is a robust practice to avoid configuration errors at deployment.
36-44:onlyActiveNodemodifier usage
Requiring node ownership and active status here ensures that only authorized and validly active nodes can access governance functions. This pattern helps maintain system integrity.contracts/src/interfaces/INodes.sol (3)
1-6: Interface declarations
Licensing, pragma version, and high-level documentation are appropriate.
7-13: Struct Node
TheNodestruct includesisActivein addition toisHealthy, making it more descriptive for node status management. This addition aligns well with the PR objective to track the active state of nodes.
25-36: Public functions adhere to interface requirements
All declared functions, includingupdateActive, are consistent with the newly introducedisActivefield. The interface covers essential ERC721 interactions likeownerOf. Overall, this interface is well-structured.contracts/src/Nodes.sol (7)
27-27:isActiveadded to Node struct
Adding theisActiveboolean enriches node management by enabling an explicit active/inactive state. This meets the PR objective for tracking network participation.
35-35:NodeCreatedevent
Emitting the newly created node state improves transparency. No issues found.
37-38:NodeActivatedandNodeDeactivatedevents
Having separate events for activation and deactivation fosters clear auditing of node states. This is helpful for dApps or external services tracking node availability.
55-56:addNodeinitializesisActiveto false
By defaultingisActivetofalse, you ensure explicit activation later, preventing accidental inclusion of new nodes as active. This is a good safety measure.
77-77: Event emission inupdateHttpAddress
You properly emit an update event whenever the node’s metadata changes, maintaining consistency with how node state changes are broadcast.
89-89:updateHealthevent emission
Continuing to emitNodeUpdatedupon health changes ensures external watchers stay informed. This aligns with the rest of the event design.
92-99:updateActivefunction
Allowing the contract owner to change a node’s active status is a crucial addition. The emittedNodeActivatedorNodeDeactivatedhelps external systems respond immediately to this status update.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
contracts/src/Nodes.sol (1)
74-92: Reevaluate restricting token transfers toonlyOwner.
Standard ERC721 usage typically allows the current token owner (rather than the contract owner) to handle transfers. RestrictingtransferFromtoonlyOwneroverrides standard expectations, which may not be desirable if node operators should freely transfer nodes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/Nodes.sol(1 hunks)contracts/src/interfaces/INodes.sol(1 hunks)
🔇 Additional comments (2)
contracts/src/interfaces/INodes.sol (1)
1-219: Interface structure is clear and consistent.
No functional or syntactical issues stand out in the error definitions, structs, events, and owner-only function signatures. The layout adheres well to recognized Solidity interface best practices.contracts/src/Nodes.sol (1)
10-22: Documentation is helpful and comprehensive.
The contract purpose and responsibilities are clearly explained, matching the interface design.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
contracts/src/Nodes.sol (2)
74-92: Consider offering a specialized transfer flow for active nodes.Automatically deactivating the node in
transferFrommay catch some operators by surprise if they intend to keep the node active under new ownership. While this approach might be intentional, providing an alternative or documenting this behavior more prominently could clarify the intended usage.
160-170: OptimizeallNodes()iteration for large node sets.Currently,
allNodes()iterates from0to_nodeCounterand checks whether a node exists. This approach may become costly with a high node count. Consider storing minted IDs in a separate array or set for more efficient lookups.Here is a conceptual snippet for reference:
function allNodes() public view returns (NodeWithId[] memory) { - NodeWithId[] memory allNodesList = new NodeWithId[](_nodeCounter); - for (uint32 i = 0; i < _nodeCounter; i++) { - uint32 nodeId = NODE_INCREMENT * (i + 1); - if (_nodeExists(nodeId)) { - allNodesList[i] = NodeWithId({nodeId: nodeId, node: _nodes[nodeId]}); - } - } - return allNodesList; + uint256[] memory mintedNodeIds = <some_stored_nodeId_array>; + NodeWithId[] memory allNodesList = new NodeWithId[](mintedNodeIds.length); + for (uint256 i = 0; i < mintedNodeIds.length; i++) { + uint256 nodeId = mintedNodeIds[i]; + allNodesList[i] = NodeWithId({nodeId: nodeId, node: _nodes[nodeId]}); + } + return allNodesList; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/Nodes.sol(1 hunks)contracts/src/interfaces/INodes.sol(1 hunks)
🔇 Additional comments (2)
contracts/src/interfaces/INodes.sol (1)
46-49: Clarify mismatch between “active” andisReplicationEnabled.Although both
isActiveandisReplicationEnabledexist in theNodestruct, the documentation does not clarify whether they represent separate states or if they should be aligned under a single “active” concept. Maintaining two flags for similar notions could lead to confusion for integrators.contracts/src/Nodes.sol (1)
116-127: Thank you for fixing the unconditionalmaxActiveNodescheck.The condition now properly applies only to activation. This ensures that deactivations do not inadvertantly revert when
maxActiveNodesis reached.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
contracts/src/Nodes.sol (3)
10-22: Enhance role documentation in contract description.The contract documentation should specify the responsibilities and permissions of each role (
DEFAULT_ADMIN_ROLEandNODE_MANAGER_ROLE).Add the following to the documentation:
/// @dev All nodes on the network periodically check this contract to determine which nodes they should connect to. -/// The contract owner is responsible for: +/// The contract has two roles: +/// - DEFAULT_ADMIN_ROLE: Can manage roles, update commission, and manage active nodes +/// - NODE_MANAGER_ROLE: Can manage node properties and transfer nodes +/// +/// Role holders are responsible for: /// - minting and transferring NFTs to node operators. /// - updating the node operator's HTTP address and MTLS certificate. /// - updating the node operator's minimum monthly fee. /// - updating the node operator's API enabled flag.
136-144: Optimize batch update operation.The current implementation processes updates one by one, which could be gas-intensive for large batches. Consider implementing a more gas-efficient batch update.
function batchUpdateActive(uint256[] calldata nodeIds, bool[] calldata isActive) external onlyRole(DEFAULT_ADMIN_ROLE) { require(nodeIds.length == isActive.length); + uint256 activeCount = _activeNodes.length(); + uint256 potentialNewActive = 0; + for (uint256 i = 0; i < nodeIds.length; i++) { + if (isActive[i] && !_activeNodes.contains(nodeIds[i])) { + potentialNewActive++; + } + } + require(activeCount + potentialNewActive <= maxActiveNodes, MaxActiveNodesReached()); for (uint256 i = 0; i < nodeIds.length; i++) { - updateActive(nodeIds[i], isActive[i]); + require(_nodeExists(nodeIds[i]), NodeDoesNotExist()); + if (isActive[i]) { + require(_activeNodes.add(nodeIds[i]), NodeAlreadyActive()); + } else { + require(_activeNodes.remove(nodeIds[i]), NodeAlreadyInactive()); + } + _nodes[nodeIds[i]].isActive = isActive[i]; + emit NodeActivateUpdated(nodeIds[i], isActive[i]); } }
193-199: Consider memory optimization for active nodes retrieval.The current implementation creates a new array and copies each node. For large sets, this could be memory-intensive.
Consider returning
NodeWithId[]instead to allow clients to selectively access node data:-function getActiveNodes() external view returns (Node[] memory activeNodes) { +function getActiveNodes() external view returns (NodeWithId[] memory activeNodes) { - activeNodes = new Node[](_activeNodes.length()); + activeNodes = new NodeWithId[](_activeNodes.length()); for (uint32 i = 0; i < _activeNodes.length(); i++) { - activeNodes[i] = _nodes[_activeNodes.at(i)]; + uint256 nodeId = _activeNodes.at(i); + activeNodes[i] = NodeWithId({ + nodeId: nodeId, + node: _nodes[nodeId] + }); } return activeNodes; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/src/Nodes.sol(1 hunks)
🔇 Additional comments (2)
contracts/src/Nodes.sol (2)
90-98: LGTM! Good security practice.The function correctly deactivates nodes before transfer and requires the
NODE_MANAGER_ROLE, which is a good security practice.
224-230: LGTM! Clean implementation of node deactivation.The helper function correctly handles the deactivation process and event emission.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/src/Nodes.sol (1)
194-201: Consider gas optimization for getActiveNodes.The function allocates and copies node data for all active nodes. For large sets of nodes, this could be gas-intensive. Consider implementing pagination or limiting the maximum number of nodes that can be retrieved at once.
- function getActiveNodes() external view returns (Node[] memory activeNodes) { + function getActiveNodes(uint256 offset, uint256 limit) external view returns (Node[] memory activeNodes) { + uint256 totalActive = _activeNodes.length(); + require(offset < totalActive, "Invalid offset"); + uint256 count = (offset + limit > totalActive) ? totalActive - offset : limit; - activeNodes = new Node[](_activeNodes.length()); + activeNodes = new Node[](count); - for (uint32 i = 0; i < _activeNodes.length(); i++) { + for (uint32 i = 0; i < count; i++) { - activeNodes[i] = _nodes[_activeNodes.at(i)]; + activeNodes[i] = _nodes[_activeNodes.at(i + offset)]; } return activeNodes; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/src/Nodes.sol(1 hunks)
🔇 Additional comments (6)
contracts/src/Nodes.sol (6)
5-8: LGTM! Good security improvement with AccessControl.The transition from
OwnabletoAccessControlenhances security by enabling fine-grained role management. The addition ofEnumerableSetfor tracking active nodes is also a good optimization.
10-22: LGTM! Well-documented contract purpose and responsibilities.The documentation clearly outlines the contract's purpose, functionality, and administrative responsibilities. This enhances maintainability and helps future developers understand the system.
60-64: LGTM! Secure role initialization in constructor.The constructor properly validates the admin address and correctly sets up both the default admin and node manager roles.
90-98: LGTM! Secure node transfer implementation.The function properly deactivates the node before transfer and requires the NODE_MANAGER_ROLE, preventing potential issues with active nodes changing ownership.
122-133: LGTM! Fixed active nodes management.The
updateActivefunction has been corrected to only checkmaxActiveNodeswhen activating a node, addressing the previous review feedback.
226-232: LGTM! Safe node deactivation implementation.The
_deactivateNodefunction safely handles node deactivation by checking existence in the active set before attempting removal, preventing unnecessary state changes and events.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
contracts/src/interfaces/INodes.sol (3)
7-13: Documentation references the old governance structure.
The doc lines mention "the contract owner" as responsible for node operations, but the actual implementation inNodes.solusesAccessControlroles (DEFAULT_ADMIN_ROLE,NODE_MANAGER_ROLE). Consider updating the doc references or clarifying that "contract owner" corresponds to addresses that have the relevant admin roles.
43-57: Clarify overlap ofisActive,isReplicationEnabled, andisApiEnabled.
Three booleans—each controlling different aspects of node functionality—can cause confusion about whether "active" nodes must also have replication or API enabled. Consider including clarifying comments or a design note to help integrators understand how these flags interact.
145-158: Consider specifying limits or guidelines forminMonthlyFee.
While leavingminMonthlyFeeflexible might be intentional, the interface does not declare any upper/lower bound or safe range. This could lead to unexpected large values if not enforced in the implementation. If a maximum is desired, consider adding it here in the interface documentation or as a dedicated requirement.contracts/src/Nodes.sol (1)
85-98: Align doc comments ontransferFromwith the access control model.
The doc states "Only the contract owner may call," but the code enforcesonlyRole(NODE_MANAGER_ROLE). Update documentation to accurately reflect this role-based requirement, ensuring no confusion about who can invoke this function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/Nodes.sol(1 hunks)contracts/src/interfaces/INodes.sol(1 hunks)
🔇 Additional comments (2)
contracts/src/Nodes.sol (2)
66-83: Revisit maximum validation forminMonthlyFee(previously discussed).
This function currently validates addresses and key parameters but lacks an upper bound check onminMonthlyFee. Past reviews noted the potential for excessively large fees. If a maximum limit is desired for safety or user protection, please add a validation.
122-133: Activation logic now properly segregates checks for activation vs. deactivation.
Thank you for gating theMaxActiveNodesReached()requirement behind the activation branch only. This improvement prevents accidental reverts upon deactivation. The implementation looks consistent with the interface.
contracts/src/Nodes.sol
Outdated
| bytes32 public constant NODE_MANAGER_ROLE = keccak256("NODE_MANAGER_ROLE"); | ||
|
|
||
| /// @dev The maximum commission percentage that the node operator can receive. | ||
| uint256 public constant MAX_BPS = 10000; |
There was a problem hiding this comment.
Great. One of the asks from our lawyers is to put bounds on any parameters in the contract, so that there is a predictable range for variable fees.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contracts/src/Nodes.sol (1)
86-95: Documentation Mismatch With Access Control
The doc block says “only the contract owner may call this,” yet the function is restricted byNODE_MANAGER_ROLE. This could confuse maintainers or integrators who interpret “owner” differently than “managers.”Consider updating the doc to say “only addresses with the
NODE_MANAGER_ROLE” may call this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/Nodes.sol(1 hunks)contracts/src/interfaces/INodes.sol(1 hunks)
🔇 Additional comments (9)
contracts/src/interfaces/INodes.sol (5)
1-14: File Overview Is Clear
The file header and imports are well-organized, and the high-level documentation matches the PR's objective, clearly describing the purpose of managing node NFTs.
46-60: Struct Definitions Look Well-Structured
BothNodeandNodeWithIdinclude clearly documented fields. The distinction betweenisReplicationEnabled,isApiEnabled, andisActiveis explicit, resolving any confusion around how a “node being active” differs from “replication support.”
74-143: Event Declarations Are Consistent
All event parameters align with their corresponding fields in theNodestruct and the function signatures. This consistency ensures clarity when other contracts or off-chain services listen for node updates.
148-215: Function Signatures Appear Consistent With The PR Goals
All node management and configuration updates (e.g.,addNode,updateMinMonthlyFee, etc.) mirror the design goals stated in the PR, laying out an intuitive interface for node lifecycle management.
220-247: Getter Functions Provide Comprehensive Node Queries
The getters (allNodes,getNode,getActiveNodes,getActiveNodesIDs, andnodeIsActive) ensure thorough retrieval of node states. This interface design satisfies common client access patterns.contracts/src/Nodes.sol (4)
5-8: Use of AccessControl & EnumerableSet
Migrating from simple ownership to role-based access viaAccessControlEnumerableandEnumerableSetaligns with the PR’s goal of enhanced node management.
68-82: Consider Maximum Bound forminMonthlyFee
While you validate thataddressandsigningKeyPubare non-empty, there’s no upper limit check onminMonthlyFeehere (or in the struct). A node could theoretically set an excessively large fee.Add a sensible maximum (e.g.,
require(minMonthlyFee <= SOME_MAX)) to help avoid abusive scenarios.
116-121: Likewise, EnforceminMonthlyFeeBound in Update
Similarly,updateMinMonthlyFeehas no upper-bound check. Having an enforced limit ensures consistent, predictable fee ranges, as requested by project stakeholders.function updateMinMonthlyFee(uint256 nodeId, uint256 minMonthlyFee) external onlyRole(NODE_MANAGER_ROLE) { require(_nodeExists(nodeId), NodeDoesNotExist()); + require(minMonthlyFee <= SOME_MAX, "minMonthlyFee too large"); _nodes[nodeId].minMonthlyFee = minMonthlyFee; emit MinMonthlyFeeUpdated(nodeId, minMonthlyFee); }
224-231: Node Deactivation Logic
Deactivating a node before transfer and removing it from_activeNodeshelps maintain consistency of the active node set. This addresses previous concerns about leftover active state during ownership changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
contracts/test/Nodes.sol (4)
17-19: Addresses initialization is straightforward.
Usingvm.randomAddress()is fine for testing. Keep in mind that any repeated calls could generate duplicates, though unlikely.
34-48: Comprehensive checks for node properties.
The test verifies the newly introduced fields (isActive,isApiEnabled, etc.) and the newminMonthlyFeeparameter. Consider also verifying related events if the contract emits them.
164-164: Plan to test safe transfers.
The TODO indicates missing coverage for safe transfers. Completing these tests will ensure compliance with ERC721 standards.Would you like help drafting these safe transfer tests?
166-182: Additional coverage for retrieval methods.
test_allNodesandtest_activeNodesconfirm advanced listing functionality. Consider verifying the returned struct contents as well (e.g., verifying operators or keyed fields), not just the array length.contracts/test/utils/Utils.sol (1)
19-31:_genRandomIntis suitable for testing, not production.
No cryptographic security is provided here, but for local or unit tests, this is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/script/DeployNodeRegistry.s.sol(1 hunks)contracts/test/Nodes.sol(1 hunks)contracts/test/utils/Utils.sol(1 hunks)
🔇 Additional comments (8)
contracts/test/Nodes.sol (6)
4-12: Imports look consistent.
All newly introduced imports are appropriate and necessary. No issues spotted regarding version conflicts or missing references.
14-14: Inheritance fromUtilsis beneficial.
Inheriting fromUtilsensures the utility functions are consistently available, reducing code duplication.
29-31: Constructor usage aligns with new permissions model.
InstantiatingNodeswithadminand grantingNODE_MANAGER_ROLEtomanagercorrectly reflects the AccessControl design.
68-92: Unauthorized additions are properly tested.
Reverts withDEFAULT_ADMIN_ROLEchecks confirm that only addresses with the admin role can add nodes.
101-114: Ownership transfer flow looks correct.
Requiring approval for the manager role prior totransferFromis consistent with the new permission model. Test coverage is adequate.
198-205: Random data generation in_randomNodeis acceptable for test context.
Since this is purely for testing, non-cryptographically secure randomness is fine. No concerns spotted.contracts/script/DeployNodeRegistry.s.sol (1)
12-12: Constructor argument aligns with AccessControl changes.
Passingmsg.senderintoNodesaligns with the new approach requiring an admin address at deployment.contracts/test/utils/Utils.sol (1)
33-44: Useful helpers for byte and string generation.
These functions reduce duplication and keep tests straightforward. They look well-structured.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
contracts/src/interfaces/INodes.sol (2)
49-63: Consider bounding or documentingminMonthlyFee.
The interface supports a freely settableminMonthlyFeewith no maximum, which came up in previous reviews regarding potential extremely high fees. If you intend to limit or validate that value, mention the constraints in the doc, or remove references to a maximum if unbounded fees are acceptable.
214-217: Rename “updateIsApiEnabled” for explicit toggling clarity.
Since this function toggles the API enabled flag rather than setting it to a passed value, consider renaming it to something liketoggleIsApiEnabledorswitchApiEnabledState. This helps integrators understand that it flips the current flag rather than updating it to an arbitrary boolean.contracts/src/Nodes.sol (3)
5-7: Avoid dual imports if unnecessary.
BothAccessControlEnumerableandAccessControlare imported. If you only need functionalities from theAccessControlEnumerableextension (which already includesAccessControl), consider removing the redundantAccessControlimport for clarity.-import "@openzeppelin/contracts/access/extensions/AccessControlEnumerable.sol"; -import "@openzeppelin/contracts/access/AccessControl.sol"; +import "@openzeppelin/contracts/access/extensions/AccessControlEnumerable.sol";
86-100: Fix the documentation fortransferFrom.
The doc states “Only the contract owner may call this,” but the code usesonlyRole(NODE_MANAGER_ROLE). Update the doc to reflect that the node manager, not the owner, is authorized to perform transfers.-/// @dev Only the contract owner may call this. Automatically deactivates the node +/// @dev Only addresses with the NODE_MANAGER_ROLE may call this. Automatically deactivates the node
116-121: Revisit adding an upper bound toupdateMinMonthlyFee.
Similar toaddNode, this function updatesminMonthlyFeewith no maximum check. If large fees can harm the ecosystem, consider imposing or documenting a maximum.contracts/test/Nodes.sol (1)
168-173: Add test coverage for “updateIsApiEnabled” unauthorized caller.
While the contract enforcesmsg.senderownership, there’s no direct test verifying a revert scenario when a non-owner tries to toggle API status. Consider adding a negative test to strengthen coverage.Do you want me to help write a test method for verifying that unauthorized addresses cannot call
updateIsApiEnabledon someone else’s node?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/src/Nodes.sol(1 hunks)contracts/src/interfaces/INodes.sol(1 hunks)contracts/test/Nodes.sol(1 hunks)
🔇 Additional comments (3)
contracts/src/interfaces/INodes.sol (1)
7-14: Ensure clarity on the “active” concept vs.isReplicationEnabled.
The documentation mentions special features like replication and node property updates, but it doesn’t clarify howisActivevs.isReplicationEnabledinterrelate. If “active” is always tied to replication being enabled, please consolidate or clarify the doc comments to avoid confusion.contracts/src/Nodes.sol (1)
123-135: Consolidate active status vs. replication logic.
You maintain separate fields forisActiveandisReplicationEnabled, potentially duplicating the concept of a node being operational. As highlighted before, consider whether a single “active” definition suffices or clarify the difference in docs.contracts/test/Nodes.sol (1)
40-54: Well-structured node addition test!
This test thoroughly checks each node property set byaddNode. Great job ensuring your function logic matches the stored node fields.
|
|
||
| uint32 nodeId = nodes.addNode(operator, node.signingKeyPub, node.httpAddress); | ||
| function test_updateMinMonthlyFee() public { |
There was a problem hiding this comment.
Love seeing all these small, narrow, tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
contracts/test/Nodes.sol (1)
366-374: Consider enforcing an upper bound forminMonthlyFee.
While this test ensures the fee update mechanism works, there is no check for unreasonably large fee values. Consider adding a validation in the contract and extending the test.contracts/src/Nodes.sol (1)
72-75: Consider boundingminMonthlyFee.
No upper limit is enforced when adding or updating a node’s fee. If a practical limit is required to prevent extreme values, add a require check and an associated test.Also applies to: 121-125
contracts/src/interfaces/INodes.sol (1)
49-63: Struct design is clear, but consider bounding fees.
Mirroring the contract suggestion, you could note a maximum feasible fee in the interface documentation to guide implementers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/src/Nodes.sol(1 hunks)contracts/src/interfaces/INodes.sol(1 hunks)contracts/test/Nodes.sol(1 hunks)
🔇 Additional comments (10)
contracts/test/Nodes.sol (4)
4-15: No issues found with the new import statements and references.
41-57: Thorough test coverage for node addition.
816-820: Helper function_addNode()appears straightforward and sufficient.
833-842: Verify_genRandomIntusage.
Though safe for typical 32-bit range tests, confirm that random data is appropriate for logic requiring deterministic outcomes. Otherwise, there's no immediate concern.contracts/src/Nodes.sol (3)
90-103: Properly deactivating nodes before transfer.
Deactivating a node upon transfer helps avoid confusion over node ownership. This approach is solid and aligns with best practices.Also applies to: 239-246
113-118: Active vs. replication enabling can be confusing.
Previous suggestions have highlighted that “active” andisReplicationEnabledcoexist, but the contract uses them for separate notions. Reconcile or document this difference more explicitly for integrators.Also applies to: 127-138
128-155: Robust checks for active node limits.
Reverting if the contract tries to activate beyondmaxActiveNodesis correct. SettingnewMaxActiveNodesbelow the current active count also reverts as expected. Good defensive design.contracts/src/interfaces/INodes.sol (3)
1-14: Interface definition is comprehensive and well-documented.
53-61: Clarify “active” vs. “isReplicationEnabled” usage for integrators.
This has been raised before and remains a point of potential confusion.
77-105: Extensive event coverage for node lifecycle changes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
contracts/src/Nodes.sol (1)
72-88:⚠️ Potential issueAdd bounds for
minMonthlyFeeparameter.As requested by legal team and noted in previous reviews, parameters should have predictable ranges. The
minMonthlyFeeparameter currently lacks upper bounds validation.Add validation like:
function addNode(address to, bytes calldata signingKeyPub, string calldata httpAddress, uint256 minMonthlyFee) external onlyRole(ADMIN_ROLE) returns (uint256) { require(to != address(0), InvalidAddress()); require(signingKeyPub.length > 0, InvalidSigningKey()); require(bytes(httpAddress).length > 0, InvalidHttpAddress()); + require(minMonthlyFee <= MAX_MONTHLY_FEE, "Fee exceeds maximum allowed");
🧹 Nitpick comments (2)
contracts/src/Nodes.sol (2)
174-178: Document the toggle behavior inupdateIsApiEnabled.While the function correctly restricts access to the node owner, the toggle behavior is not immediately clear from the function name.
- function updateIsApiEnabled(uint256 nodeId) external { + /// @notice Toggles the API enabled state for a node + /// @dev Only the node owner can toggle their node's API state + /// @param nodeId The ID of the node to toggle + function updateIsApiEnabled(uint256 nodeId) external {
181-190: Optimize memory allocation ingetAllNodes.The function allocates an array for all possible nodes (including gaps), which could waste memory if there are many gaps in node IDs.
function getAllNodes() public view returns (NodeWithId[] memory allNodesList) { - allNodesList = new NodeWithId[](_nodeCounter); + // Count actual nodes first + uint256 actualCount = 0; + for (uint32 i = 0; i < _nodeCounter; i++) { + uint32 nodeId = NODE_INCREMENT * (i + 1); + if (_nodeExists(nodeId)) { + actualCount++; + } + } + // Allocate exact size needed + allNodesList = new NodeWithId[](actualCount); + uint256 currentIndex = 0; for (uint32 i = 0; i < _nodeCounter; i++) { uint32 nodeId = NODE_INCREMENT * (i + 1); if (_nodeExists(nodeId)) { - allNodesList[i] = NodeWithId({nodeId: nodeId, node: _nodes[nodeId]}); + allNodesList[currentIndex] = NodeWithId({nodeId: nodeId, node: _nodes[nodeId]}); + currentIndex++; } } return allNodesList; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/src/Nodes.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: abis
🔇 Additional comments (4)
contracts/src/Nodes.sol (4)
22-69: LGTM! Well-structured role-based access control implementation.The transition from Ownable to AccessControl with distinct roles (ADMIN_ROLE and NODE_MANAGER_ROLE) provides better granularity and flexibility in permission management. The use of EnumerableSet for tracking active nodes is a gas-efficient choice.
127-156: LGTM! Fixed themaxActiveNodescheck.The activation logic has been improved:
maxActiveNodesis now only checked when activating nodes- Batch updates properly validate input lengths
- Max nodes updates ensure the new limit accommodates current active nodes
157-171: LGTM! Well-validated configuration functions.The configuration functions include proper validation:
- Commission percentage is bounded by MAX_BPS
- Base URI format is validated including trailing slash requirement
226-257: LGTM! Well-implemented helper functions.The helper functions are robust:
- Node existence check uses ERC721's _ownerOf
- Node deactivation properly handles the active nodes set
- Interface support is correctly implemented for all parent contracts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/solidity.yml (1)
82-83: Simplified Cache Restoration Path
The cache restoration step now uses a single directory (contracts) instead of multiple individual paths. This streamlines the cache process and reduces potential configuration complexity. Please verify that this broader cache path doesn’t unintentionally include files that might slow down cache save/restore times.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/solidity.yml(2 hunks)
🔇 Additional comments (1)
.github/workflows/solidity.yml (1)
180-181: CleanedabigenCommand Formatting
The removal of the extra newline in theabigencommand offers cleaner formatting and consistency in the workflow configuration.
fbac
force-pushed
the
contracts/add-active-metadata
branch
from
3c67fd4 to
79c6dbd
Compare
fbac
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
contracts/dev/generate (1)
54-54: Add nodes scanning or generation tests.Good job adding
"NodesV2"to the default list. If there aren't tests or build validations covering"NodesV2"specifically, consider adding them to ensure reliable integration.contracts/src/NodesV2.sol (4)
23-26: Assess role granularity and privileges.You define two roles:
ADMIN_ROLEandNODE_MANAGER_ROLE. This is clear but be sure your design considers all future expansions. For any advanced node operations that may lie outside standard administration, consider a role with more granular privileges.
90-104: Confirm overriding transferFrom meets ERC721 expectations.
transferFromis restricted toNODE_MANAGER_ROLE. This is a deliberate override. Validate that third-party marketplaces or token controllers dependent on standard transfers are not blocked from transferring these NFTs.
173-179: Add a cooldown or permission check for updateIsApiEnabled if necessary.Allowing node operators to toggle
isApiEnabledmight be abused for rapid toggling if not carefully monitored. Consider whether you need a rate-limiting mechanism or additional checks.
203-210: Optimize loop or consider direct iteration for getActiveNodes.The loop uses
_activeNodes.length()and_activeNodes.at(i)repeatedly. This is acceptable for smaller sets, but if usage scales, you might prefer returning_nodesvia other means.contracts/test/Nodes.sol (3)
583-609: Consider adding edge cases for commission percent tests.The commission percent tests could be more comprehensive. Consider adding tests for:
- Edge case of 0% commission
- Edge case of 100% commission (10000)
- Decimal point precision validation
788-810: Consider adding more security-focused tests.While the current security tests are good, consider adding:
- Tests for role transfer scenarios
- Tests for potential reentrancy vulnerabilities
- Tests for event emission in role management operations
833-842: Consider enhancing _randomNode with more diverse test data.The _randomNode function could be improved by:
- Adding randomization for boolean fields
- Varying the length of generated strings and bytes
- Adding boundary testing values
function _randomNode() internal view returns (INodes.Node memory) { return INodes.Node({ signingKeyPub: _genBytes(32), httpAddress: _genString(32), - isReplicationEnabled: false, - isApiEnabled: false, - isActive: false, + isReplicationEnabled: _genRandomBool(), + isApiEnabled: _genRandomBool(), + isActive: _genRandomBool(), minMonthlyFee: _genRandomInt(100, 10000) }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/solidity.yml(4 hunks)contracts/dev/generate(1 hunks)contracts/pkg/nodes/Nodes.go(1 hunks)contracts/script/DeployNodeRegistry.s.sol(0 hunks)contracts/src/Nodes.sol(1 hunks)contracts/src/NodesV2.sol(1 hunks)contracts/test/Nodes.sol(1 hunks)
💤 Files with no reviewable changes (1)
- contracts/script/DeployNodeRegistry.s.sol
✅ Files skipped from review due to trivial changes (1)
- contracts/src/Nodes.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/solidity.yml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: abis
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (10)
contracts/pkg/nodes/Nodes.go (1)
48-48: No changes required for the updated bytecode.This line reflects autogenerated updates to the contract bytecode. Unless there’s a specific need to tailor the bytecode (e.g., for debugging), no changes are needed here.
contracts/src/NodesV2.sol (7)
1-22: Ensure correctness of documentation remarks.The introductory comments accurately summarize the purpose of the Nodes Registry. Verify that node operators are fully informed about the newly introduced functionalities and the 100-era ID increments.
39-40: Validate maxActiveNodes updates in external calls.
maxActiveNodesis mutable. Confirm that any external integrations (e.g., front-end apps, off-chain scripts) gracefully handle changes to the limit of active nodes without unexpected behavior.
59-69: Constructor roles are well-defined.Granting both
ADMIN_ROLEandNODE_MANAGER_ROLEto_initialAdminis straightforward. Just confirm the contract's security posture remains robust when the default admin can manage the entire system.
71-88: Consider reentrancy or malicious mint checks in addNode.While no immediate reentrancy is apparent, ensure untrusted receivers (
to) do not disrupt safe minting. For example,_mintcan trigger external callbacks iftois a contract implementing ERC721Receiver. Usually safe, but worth verifying.
127-138: Check side effects before node activation.Activating or deactivating a node triggers
_activeNodes.addor.remove. Ensure that any dependent on-chain or off-chain references to node statuses also handle these real-time changes and that duplication or concurrency issues are avoided.
227-233: Keep _nodeExists private usage consistent with node checks.The helper
_nodeExistsis succinct. Confirm that all external or public calls meant to verify node existence are indeed using it or an equivalent check, especially in external-facing functions.
249-258: Inherited interface support is correct.Your
supportsInterfaceoverride callssuper.supportsInterface(interfaceId)forERC721,IERC165, andAccessControlDefaultAdminRules. This correctly ensures contract compliance across multiple standards.contracts/test/Nodes.sol (2)
1-35: LGTM! Well-structured test setup.The contract setup is comprehensive with proper role initialization and clear variable declarations.
1-848: Excellent test coverage and organization!The test suite is well-structured with comprehensive coverage of functionality, access control, and error cases. The organization into clear sections with descriptive comments makes the code very maintainable.
Closes #542 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new deployment workflow for the node registry that automatically produces a JSON summary of deployment data. - Expanded the environment configuration with additional output parameters. - **Refactor** - Updated validations in the deployment process to ensure necessary credentials are provided. - Streamlined the contract initialization process for a more flexible and robust deployment experience. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contracts/script/DeployNodeRegistryV2.s.sol (1)
34-50: Consider adding version information to serialized data.Since this is a V2 deployment, it would be helpful to include version information in the serialized output for better tracking and management.
function _serializeDeploymentData() internal { string memory parent_object = "parent object"; string memory addresses = "addresses"; string memory addressesOutput; addressesOutput = vm.serializeAddress(addresses, "XMTPNodeRegistryDeployer", deployer); addressesOutput = vm.serializeAddress(addresses, "XMTPNodeRegistryInitialAdmin", admin); addressesOutput = vm.serializeAddress(addresses, "XMTPNodeRegistry", address(nodes)); string memory finalJson; finalJson = vm.serializeString(parent_object, addresses, addressesOutput); finalJson = vm.serializeUint(parent_object, "deploymentBlock", block.number); finalJson = vm.serializeUint(parent_object, "latestUpgradeBlock", block.number); + finalJson = vm.serializeString(parent_object, "version", "2.0.0"); writeOutput(finalJson, XMTP_NODE_REGISTRY_OUTPUT_JSON); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
contracts/script/DeployGroupMessages.s.sol(1 hunks)contracts/script/DeployIdentityUpdates.s.sol(1 hunks)contracts/script/DeployNodeRegistryV1.s.sol(1 hunks)contracts/script/DeployNodeRegistryV2.s.sol(1 hunks)contracts/script/utils/Environment.sol(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- contracts/script/DeployNodeRegistryV1.s.sol
⏰ 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: Push Docker Images to GitHub Packages (xmtpd)
🔇 Additional comments (4)
contracts/script/utils/Environment.sol (1)
9-9: LGTM!The new constant follows the established naming convention and its value aligns with the contract's purpose.
contracts/script/DeployGroupMessages.s.sol (1)
23-24:✅ Verification successful
Verify the intentional removal of admin contract check.
The admin contract check has been removed while a private key validation has been added. Please confirm if removing the admin contract check is intentional as part of the breaking changes.
🏁 Script executed:
#!/bin/bash # Description: Check if admin contract validation exists in other deployment scripts. # Test: Search for admin contract validation patterns. rg -A 5 $'admin.*contract|contract.*admin'Length of output: 2401
Confirmation: Admin Contract Check Removal is Intentional
After reviewing the repository, we found that admin validations are now handled within the Solidity contracts (e.g., in
contracts/src/GroupMessages.solandcontracts/src/IdentityUpdates.sol, whererequire(admin != address(0), ZeroAdminAddress())enforces a non-zero admin). In the deployment script (contracts/script/DeployGroupMessages.s.sol), the check has been replaced with a validation forPRIVATE_KEY, ensuring that the deployment is only attempted with a properly set private key.
- The deployment script now uses:
require(privateKey != 0, "PRIVATE_KEY not set");- Admin verification is performed in the contract initialization rather than during deployment.
- Similar admin configuration settings are documented (e.g., in
contracts/README.md), indicating that this change aligns with the overall design.It appears that the removal of the admin contract check is an intentional breaking change. No further action is required on this point unless you need adjustments to related documentation or environment configurations.
contracts/script/DeployIdentityUpdates.s.sol (1)
23-24: Similar changes as in DeployGroupMessages.s.sol.The changes mirror those in DeployGroupMessages.s.sol, maintaining consistency across deployment scripts.
contracts/script/DeployNodeRegistryV2.s.sol (1)
1-32: LGTM!The deployment script follows the established pattern and includes appropriate validations. The structure is consistent with other deployment scripts in the codebase.
Closes #540
Note: This is a breaking change. Go tests are not expected to pass.
Summary by CodeRabbit
New Features
Style
Chores