feat: integrate new nodes contract with xmtpd #550
Conversation
WalkthroughThis pull request removes references to the deprecated Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DeployScript as DeployNodeRegistry.s.sol
participant NodesContract as Nodes.sol
User->>DeployScript: Execute deployment script (no arguments)
DeployScript->>NodesContract: new Nodes(admin)
NodesContract-->>DeployScript: Return deployed instance
DeployScript->>DeployScript: Verify non-zero address
sequenceDiagram
participant Admin
participant NodesContract as Nodes.sol
Admin->>NodesContract: Call addNode(to, signingKeyPub, httpAddress, minMonthlyFee)
Note right of NodesContract: Verify ADMIN_ROLE<br>and process node addition
NodesContract-->>Admin: Emit event and store node data
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24) ✨ Finishing Touches
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 (4)
contracts/src/Nodes.sol (4)
90-103: Revisit the restrictive approach to NFT transfers.
This override only allows addresses withNODE_MANAGER_ROLEto invoketransferFrom. This deviates from typical ERC721 patterns where owners or approved operators can transfer tokens. Confirm that restricting transfers exclusively to node managers is intended.
105-112: Evaluate ownership vs. manager authority for updating HTTP addresses.
Currently, onlyNODE_MANAGER_ROLEcan update the node’shttpAddress. If node operators are expected to manage their own addresses, consider extending permissions or adding a dedicated function for owners.
132-142: Consider empowering node owners to update replication status.
Currently, replication enablement is restricted to theNODE_MANAGER_ROLE. If your design expects node operators to self-manage replication, consider a flow similar to the API toggle. Otherwise, if central management is intentional, this is fine.
163-172: Batch activation works well for small sets; watch for gas limits.
This loop callsupdateActiveindividually for each node. For large arrays, you might approach block gas limits. If you foresee onboarding large numbers of nodes, consider chunking or another scalable approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
contracts/dev/generate(1 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/src/interfaces/INodes.sol(3 hunks)contracts/test/Nodes.sol(12 hunks)
💤 Files with no reviewable changes (2)
- contracts/src/NodesV2.sol
- contracts/script/DeployNodeRegistryV1.s.sol
🔇 Additional comments (24)
contracts/dev/generate (1)
54-54: Updating default contracts to remove NodesV2The removal of "NodesV2" from the default contracts list aligns with the PR objective of integrating the new Nodes contract. This change ensures that only the current contract versions are generated when no specific contracts are provided as arguments.
contracts/script/DeployNodeRegistry.s.sol (3)
8-8: Updated import to use new Nodes contractThe import has been correctly updated to use the new Nodes contract instead of NodesV2.
11-11: Variable type updated to match new contractThe variable type has been properly updated from NodesV2 to Nodes.
26-26: Instantiation updated to use new Nodes contractThe instantiation has been correctly updated to use the new Nodes contract.
contracts/test/Nodes.sol (7)
7-7: Updated import to use new Nodes contractThe import has been correctly updated to use the new Nodes contract instead of NodesV2.
17-17: Variable type updated to match new contractThe variable type has been properly updated from NodesV2 to Nodes.
29-29: Instantiation updated to use new Nodes contractThe instantiation has been correctly updated to use the new Nodes contract.
413-415: Node prerequisites for activation addedImplementing the PR requirement that nodes must have both API and replication enabled to be considered active.
422-428: Added test for invalid node configurationThis new test verifies that a node cannot be activated without having both API and replication enabled, which aligns with the PR requirements.
489-489: Updated function return signatureThe return type of
_addMultipleNodesnow includes operators, improving the test's ability to manage node operators for API enabling.
865-871: Added helper function to enable nodesThis helper function simplifies the process of enabling both replication and API for nodes, making tests more maintainable.
contracts/src/interfaces/INodes.sol (3)
19-30: Error declarations reorganized and expandedNew error declarations have been added to support the enhanced node configuration requirements, properly handling validation for node states.
234-235: Updated return parameter name for clarityThe return parameter name has been changed from
allNodesListtoallNodesfor better readability.
249-249: Updated return type for getActiveNodes()The return type has been changed from
Node[] memorytoNodeWithId[] memory, aligning with the PR objective to enhance how active nodes are represented by including their IDs in the return structure.contracts/src/Nodes.sol (10)
5-7: Consider pinning dependencies to specific versions for security.
The imports from OpenZeppelin are excellent choices, but it may be safer to pin them to explicit versions in your package configuration (e.g., via a lockfile or explicit version constraints) to protect against upstream changes.
59-69: Verify the 2-day admin delay is appropriate for your use case.
TheAccessControlDefaultAdminRules(2 days, _initialAdmin)constructor call adds a 2-day timelock for certain admin changes. Ensure this delay aligns well with your security and operational requirements.
71-88: Validate the default inactive state for newly added nodes.
Upon creation, nodes are added withisActive = false,isApiEnabled = false, andisReplicationEnabled = false. Make sure that your onboarding flow expects these defaults before any activations.
120-130: Owner toggle for API enablement is consistent with a self-managed design.
The node operator (owner) can independently enable or disable the API. This matches the intent of letting node owners control live traffic. The logic to deactivate the node when disabling API is also clear and sound.
144-161: Active node criteria align with PR objectives.
Requiring both API and replication to be enabled for activation is consistent with the stated goal of only recognizing nodes that fully meet availability criteria. Logic and event emissions look solid.
196-206: Verify handling of burned or uninitialized node slots.
getAllNodesallocates an array of size_nodeCounterand conditionally fills entries. If some node IDs were never minted or later burned, those entries remain defaulted to zero. Confirm whether returning partial or zeroed data in the result is acceptable.
219-229: Efficient retrieval of active nodes.
This function matches the PR requirement by listing only actively tracked nodes. The fallback to skip non-existent nodes is a safe guard.
253-256: Customized base URI.
Overriding_baseURIis straightforward and aligns with standard ERC721 best practices for metadata, assuming the final string ends with/.
258-266: Deactivation logic cleanly updates active status.
The_deactivateNodehelper ensures the node is removed from_activeNodesand setsisActiveto false consistently. This is a robust way to maintain correct membership.
268-276: Interface support correctly delegated to parent contracts.
OverridingsupportsInterfacewhile deferring tosuperensures compatibility withERC721,IERC165, and Role-based access. This is a compliant approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
contracts/script/DeployNodeRegistry.s.sol (1)
26-27: Updated constructor call and added deployment validation.The contract instantiation has been updated to use the Nodes constructor and includes a validation check to ensure deployment was successful. This is a good addition for error handling.
Consider adding more specific error messages to indicate what part of the deployment failed:
-require(address(nodes) != address(0), "Nodes deployment failed"); +require(address(nodes) != address(0), "Nodes deployment failed: address is zero");contracts/test/Nodes.sol (1)
865-871: Added helper function to streamline node enablement.The new
_enableNodeshelper function reduces code duplication by centralizing the logic for enabling both replication and API on multiple nodes. This is a good refactoring that improves maintainability.Consider adding a comment explaining the relationship between replication, API, and node activation to make the test code more self-documenting:
function _enableNodes(address[] memory operators, uint256[] memory nodeIds) internal { + // Both replication and API must be enabled for a node to be active for (uint256 i = 0; i < nodeIds.length; i++) { nodes.updateIsReplicationEnabled(nodeIds[i], true); vm.prank(operators[i]); nodes.updateIsApiEnabled(nodeIds[i]); } }contracts/src/Nodes.sol (3)
10-22: Update doc comments to reflect role-based restrictions instead of “contract owner.”
Some references still mention "the contract owner" rather than the assigned roles (ADMIN_ROLEorNODE_MANAGER_ROLE). This can cause confusion when reading the documentation.
90-103: Clarify doc vs. code mismatch for node transfers.
The doc says “only the contract owner may call this,” but the code usesonlyRole(NODE_MANAGER_ROLE). Update the doc comment to match the role-based approach. Otherwise, the logic for deactivation on transfer looks correct.
196-206: Potential high gas usage withgetAllNodes()for large nodeCounter.
This function returns a dynamically sized array and iterates over all node IDs. Consider usage patterns carefully or break the result into paginated calls if the number of nodes grows very high.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
contracts/dev/generate(1 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/src/interfaces/INodes.sol(3 hunks)contracts/test/Nodes.sol(12 hunks)
💤 Files with no reviewable changes (2)
- contracts/script/DeployNodeRegistryV1.s.sol
- contracts/src/NodesV2.sol
🔇 Additional comments (38)
contracts/dev/generate (1)
54-54:✅ Verification successful
Default contract list updated to remove NodesV2.
This change aligns with the PR objective of transitioning to the new Nodes contract and removing references to the deprecated NodesV2 contract.
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to NodesV2 in the codebase # Look for NodesV2 references echo "Searching for remaining NodesV2 references:" rg "NodesV2" --type-not scriptLength of output: 158
Please execute the above script to verify that no unwanted references to
NodesV2remain in the codebase. Once you’ve confirmed there are no lingering references, you can update the review comment accordingly.
Updated Review Comment:
Default contract list updated to remove deprecated NodesV2 references.
This change aligns with the PR objective of transitioning to the new Nodes contract by excluding the outdated NodesV2 from the default binding generation. However, the automated search initially encountered an error with file type filtering. Please manually verify (or re-run the provided script) that there are no remaining references toNodesV2across the repository.
🏁 Script executed:
[run_scripts] #!/bin/bash # Re-run search without excluding any file type to check for 'NodesV2' references in the repository. echo "Re-checking for remaining NodesV2 references:" rg "NodesV2"Length of output: 65772
Action: Approved removal of NodesV2 from the default contract list in the generate script.
The update in
contracts/dev/generate(line 54) correctly replaces the previous default list by omittingNodesV2, aligning with the PR’s objective of transitioning to the new Nodes contract. Although the repository still contains references toNodesV2(notably incontracts/pkg/nodesv2/NodesV2.go), these appear to be maintained for backwards compatibility and do not affect the default generation of bindings.Please confirm that the legacy references are intentional and that this change won’t inadvertently disrupt any existing functionality.
contracts/script/DeployNodeRegistry.s.sol (2)
8-8: Updated import to use Nodes contract.This change correctly imports the updated Nodes contract instead of the deprecated NodesV2 contract, aligning with the PR objective of integrating the new Nodes contract.
11-11: Updated variable type to use Nodes contract.The variable declaration has been updated to use the Nodes contract type instead of NodesV2, maintaining consistency with the import change.
contracts/test/Nodes.sol (4)
7-7: Updated import and variable declarations to use Nodes contract.The test file has been properly updated to use the new Nodes contract instead of the deprecated NodesV2 contract. All import statements and variable declarations have been correctly modified.
Also applies to: 17-17, 29-29
422-428: Added test case for invalid node configuration.A new test case has been added to verify that nodes cannot be activated unless both replication and API are enabled. This aligns with the PR objective that "the
updateActive()function now mandates that a node must have both API and replication enabled to be considered active."
413-415: Updated test functions to enable replication and API before activation.Test cases have been updated to ensure that nodes have both replication and API enabled before they are activated. This correctly reflects the new requirement specified in the PR objectives.
Also applies to: 469-471, 586-589
749-749: Updated return type to use NodeWithId instead of Node.The
getActiveNodesfunction now returns an array ofNodeWithIdinstead ofNode, which aligns with the PR objective of enhancing the way active nodes are represented.contracts/src/interfaces/INodes.sol (2)
19-20: Added new error types for enhanced validation.New error types have been added to support additional validation in the contract:
InvalidCommissionPercent: For commission percentage validationInvalidInputLength: For validating input array lengthsInvalidNodeConfig: For validating node configuration (likely related to replication and API requirements)InvalidURI: For URI format validationThese additions improve the contract's error handling capabilities.
Also applies to: 25-26, 28-29, 34-35
249-249:✅ Verification successful
Updated getActiveNodes return type to use NodeWithId.
The
getActiveNodesfunction now returns an array ofNodeWithIdinstead ofNode. This change enhances the representation of active nodes by including their IDs directly in the returned data structure, aligning with the PR objective.
🏁 Script executed:
#!/bin/bash # Description: Check for any code that may still be using the old return type # Look for places that call getActiveNodes() echo "Checking for callers of getActiveNodes():" rg "getActiveNodes\(\)" -A 3 -B 1 --type solLength of output: 167
Please run the above script to verify if any callers of
getActiveNodes()are still relying on the old return type. Once we confirm there are no legacy usages (or if there are, we can address them), we can finalize the review comment.
🏁 Script executed:
[run_scripts] #!/bin/bash # Updated search for callers of getActiveNodes() in Solidity files (using glob pattern) echo "Searching for callers of getActiveNodes() across Solidity files:" rg "getActiveNodes\(\)" -A 3 -B 3 --iglob '*.sol'Length of output: 2268
Active nodes return type update verified as consistent
- The interface in
contracts/src/interfaces/INodes.solnow declaresgetActiveNodes()to returnNodeWithId[], matching the new design.- The implementation in
contracts/src/Nodes.solcorrectly constructs and returns an array ofNodeWithId.- Test contracts in
contracts/test/Nodes.solare updated to expectINodes.NodeWithId[], ensuring full consistency across the codebase.No legacy usage of the old
Nodetype was found. Please proceed with this change.contracts/src/Nodes.sol (29)
5-7: No issues with updated imports.
Declarations seem warranted, aligning with the contract’s new role-based access control and enumerable sets.
25-26: Role definitions look good.
Establishing dedicated roles for admin and node management enhances clarity and security.
28-29: Max basis points constant is appropriate.
Using 10,000 BPS (basis points) is a common pattern for representing percentages in Solidity.
34-35: Base URI variable is clearly defined.
Storing_baseTokenURIas a private string is standard, ensuring consistent metadata references.
37-39: DefaultmaxActiveNodesis set to 20.
This is a reasonable starting point for node limits.
41-44: Node counter usage is straightforward.
Incrementing node IDs by a fixed offset simplifies ID generation and identification.
46-50: Mappings and active node sets are properly declared.
Using anEnumerableSetfor active node IDs is a sensible approach to ensure membership checks and iteration.
52-58: Commission percent storage in BPS is standard.
No immediate concerns regarding how this value is stored or referenced.
59-69: Constructor logic is consistent.
It checks_initialAdminfor zero address and grants the correct roles. The 2-day delay fromAccessControlDefaultAdminRulesis a prudent choice.
71-88: Node minting appears correct.
The function enforces valid inputs, mints the NFT, and initializes node state properly (default disabled flags).
105-111: HTTP address updates look fine.
Checks the node’s existence and ensures non-empty addresses.
113-118: Minimum monthly fee updates are straightforward.
No issues found.
120-130: API enable toggle is well-implemented.
Ensures only the node owner can toggle API. If disabled, the node is deactivated.
132-142: Replication enablement logic is correct.
Checking the node’s existence, updating the flag, and deactivating if disabled is aligned with the new design.
144-161: Activity updates validate replication/API requirements.
The function properly enforces the maximum active node count and toggles membership in_activeNodes.
163-172: Batch activity updates are efficient.
Looping over arrays is straightforward; no immediate concerns.
174-179: New maximum active nodes logic is sound.
Requires new max to exceed current active set length, preventing accidental forced deactivations.
181-186: Node operator commission restrictions look good.
Bounds are enforced without extraneous logic.
188-194: Setting the base URI is validated.
Requiring a trailing slash is a good practice for NFT metadata.
208-211: Retrieves total node count.
Straightforward and cost-effective.
213-218: Single-node retrieval is correct.
Properly checks for existence and reverts if absent.
219-229: Active node retrieval is consistent.
Iterates through_activeNodes, which is typically capped bymaxActiveNodes. No immediate performance concerns unlessmaxActiveNodesis set too high.
231-234: Active node IDs array retrieval is correct.
No issues found.
236-239: Active node count retrieval is correct.
This is a simple wrapper over_activeNodes.length().
241-244: Checks if a given node ID is active.
Makes good use ofEnumerableSetmembership queries.
246-251: Private helper_nodeExistsis correct.
It simply checks ERC721 ownership, which is straightforward.
253-256: Override_baseURI()logic is standard.
Returns_baseTokenURI, as expected for metadata.
258-266:_deactivateNodehandles set removal and updates.
Properly toggles_nodes[nodeId].isActiveand emits the corresponding event.
268-277: Interface support override is in line with OpenZeppelin patterns.
Combines the relevant interfaces for ERC721 and AccessControl.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
pkg/config/cliOptions.go (1)
34-49: Missing isApiEnabled field in UpdateApiEnabledOptionsUnlike
UpdateActiveOptionsandUpdateReplicationEnabledOptions, theUpdateApiEnabledOptionsstruct doesn't have a boolean field to specify whether API should be enabled or disabled. This creates an inconsistency between the option structs.Consider adding an
IsApiEnabledboolean field for consistency:type UpdateApiEnabledOptions struct { NodeId int64 `long:"node-id" description:"NodeId to update"` OperatorPrivateKey string `long:"operator-private-key" description:"Private key of the operator to update the node" required:"true"` + IsApiEnabled bool `long:"enable" description:"Whether the node is API enabled"` }pkg/registry/contractRegistry_test.go (1)
139-146: Non-deterministic test usingrand.Intn(1000).
While this does test varying Node IDs, consider using a fixed or pseudo-random seed to ensure reproducible test runs. Otherwise, it might occasionally mask or produce timing-based flakes.cmd/cli/main.go (1)
215-252: API enablement logic.
The functionupdateApiEnabledmirrors theupdateActivepattern. This is clean and consistent. Consider unit testing these functions if not already covered elsewhere.pkg/blockchain/registryAdmin.go (2)
60-103: UpdatedAddNodemethod properly returns thenodeId.
The logic to parse transaction logs for theNodeIdis correct. Consider clarifying in the function docstring that this method blocks until the transaction is confirmed.
105-134:UpdateActivetransaction call.
This properly checks for a signer, constructs the transaction, and blocks until confirmed. Be mindful that an on-chain confirmation might exceed 2 seconds in certain networks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
cmd/cli/cli_test.go(1 hunks)cmd/cli/main.go(8 hunks)contracts/dev/deploy-local(1 hunks)contracts/dev/lib/common(1 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/interface.go(1 hunks)pkg/blockchain/registryAdmin.go(3 hunks)pkg/blockchain/registryAdmin_test.go(3 hunks)pkg/config/cliOptions.go(1 hunks)pkg/mocks/registry/mock_NodesContract.go(2 hunks)pkg/registry/contractRegistry.go(3 hunks)pkg/registry/contractRegistry_test.go(4 hunks)pkg/registry/interface.go(1 hunks)pkg/registry/node.go(5 hunks)pkg/server/server_test.go(2 hunks)pkg/sync/syncWorker.go(1 hunks)pkg/testutils/config.go(3 hunks)pkg/testutils/contracts.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cmd/cli/cli_test.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Test (Node)
🔇 Additional comments (48)
contracts/dev/deploy-local (1)
32-32: Simplified deployment command for nodes.The command for deploying nodes has been simplified by removing the source file and contract name parameters. This change aligns with the updated deployment process in the common library script.
pkg/server/server_test.go (1)
98-98: Field name change fromIsHealthytoIsActive.The change from
IsHealthytoIsActivealigns with the updated node status determination in the new Nodes contract, where a node is considered active only when both API and replication are enabled.Also applies to: 105-105, 234-234
pkg/sync/syncWorker.go (1)
175-175: Updated condition to checkIsActiveinstead ofIsHealthy.The condition for determining whether to handle an unhealthy node has been updated to check the node's active status instead of its health status. This change is consistent with the updated node status model in the new Nodes contract.
contracts/dev/lib/common (1)
35-42: Improved deployment process for Nodes contract.The deployment process has been significantly enhanced with the following improvements:
- Now using
forge scriptwith a dedicated deployment script instead offorge create- Added proper error handling with clear error messages
- Updated output message to reference a consistent file name
These changes make the deployment process more robust and maintainable.
pkg/testutils/contracts.go (1)
121-121: Updated contract deployment to use role-based access control.The additional
auth.Fromparameter aligns with the Nodes contract refactoring that now uses role-based access control instead of the previous Ownable pattern. The sender address is now explicitly set as the admin during deployment.pkg/registry/interface.go (1)
13-14: Interface updated to reflect new contract capabilities.The interface changes appropriately implement the PR objectives by:
- Renaming
AllNodestoGetAllNodesfor consistent method naming- Changing the return type to
[]nodes.INodesNodeWithIdfor better abstraction- Adding the new
GetActiveNodesmethod to support the enhanced node filtering logicThis change aligns with the PR objective of returning an array of
NodeWithIDand enforcing node activity requirements.dev/local.env (1)
13-13:❓ Verification inconclusive
Updated contract address source to use new registry structure.
The environment variable now correctly references the new contract location in the
XMTPNodeRegistry.jsonfile, maintaining compatibility with the contract deployment changes.
🏁 Script executed:
#!/bin/bash # Check if XMTPNodeRegistry.json exists and contains the expected structure if [ -f contracts/config/anvil_localnet/XMTPNodeRegistry.json ]; then echo "File exists, checking content structure..." jq -e '.addresses.XMTPNodeRegistry' contracts/config/anvil_localnet/XMTPNodeRegistry.json > /dev/null if [ $? -eq 0 ]; then echo "✅ XMTPNodeRegistry.json contains the expected structure with 'addresses.XMTPNodeRegistry' field" else echo "❌ XMTPNodeRegistry.json exists but doesn't contain the expected structure" jq '.' contracts/config/anvil_localnet/XMTPNodeRegistry.json | head -20 fi else echo "❌ XMTPNodeRegistry.json file not found" ls -la contracts/config/anvil_localnet/ fiLength of output: 275
Attention: Verify the Contract Registry File Location
The updated environment variable in
dev/local.envnow references the new contract registry structure via:XMTPD_CONTRACTS_NODES_ADDRESS="$(jq -r '.addresses.XMTPNodeRegistry' ${ANVIL_SCRIPTS_OUTPUT}/XMTPNodeRegistry.json)" # Built by contracts/dev/deploy-localHowever, the automated check did not find a file at the expected location (
contracts/config/anvil_localnet/XMTPNodeRegistry.json). This raises a couple of possibilities:
- The file may be generated in a different location than expected, with
${ANVIL_SCRIPTS_OUTPUT}pointing elsewhere.- There might be a misconfiguration where the file path in the environment variable no longer matches the actual file placement after the latest deployment changes.
Action Items:
- Manual Verification: Confirm that the deployment process generates
XMTPNodeRegistry.jsoncorrectly and that${ANVIL_SCRIPTS_OUTPUT}points to the proper directory.- File Location Update: If the file is located elsewhere, update the file path in
dev/local.envaccordingly.Please verify these points to ensure compatibility with the new contract deployment changes.
dev/baked/Dockerfile (2)
32-32: Updated contract address source to use new registry structure.This change keeps the Dockerfile in sync with the environment variable changes in
dev/local.env, ensuring consistent behavior between development environments.
40-40: Updated maintainer contact information.The maintainer label has been updated to reflect the current organizational contact.
dev/register-local-node (4)
8-8: LGTM: Added operator private key exportThe addition of the
OPERATOR_PRIVATE_KEYenvironment variable properly reuses the existingANVIL_ACC_1_PRIVATE_KEYfor node operations.
10-17: Good enhancement: Capturing node ID from registration responseThe script now captures the registration response and extracts the node ID using
jq, which is needed for the subsequent operations. This improvement makes the script more robust.
19-32: Well-implemented node activation workflowThe additional commands properly implement the new node activation workflow according to the PR requirements. The script now:
- Updates API enabled status with operator credentials
- Enables replication with admin credentials
- Activates the node with admin credentials
This sequence correctly implements the requirement that a node must have both API and replication enabled to be considered active.
33-33: Nice addition: Success message with node IDThe success message clearly indicates that the node has been registered and activated, including the node ID for reference.
dev/register-local-node-2 (4)
9-9: LGTM: Added operator private key exportThe addition of the
OPERATOR_PRIVATE_KEYenvironment variable properly reuses the existingANVIL_ACC_2_PRIVATE_KEYfor node operations, maintaining consistency with the first node registration script.
11-18: Good enhancement: Capturing node ID from registration responseThe script now captures the registration response and extracts the node ID using
jq, which is needed for the subsequent operations. This matches the approach used in the first node registration script.
20-32: Well-implemented node activation workflowThe additional commands properly implement the new node activation workflow according to the PR requirements. The sequence of operations matches that of the first script, maintaining a consistent approach to node activation across different nodes.
34-34: Nice addition: Success message with node IDThe success message clearly indicates that the node has been registered and activated, including the node ID for reference.
pkg/registry/node.go (3)
6-6: Added necessary import for new field typeThe addition of the
math/bigimport is required for the newMinMonthlyFeefield type.
20-21: Updated node status concept and added fee fieldThe change from
IsHealthytoIsActivealigns with the PR objectives where a node must have both API and replication enabled to be considered active. The addition ofMinMonthlyFeesuggests a new fee structure is being implemented.
46-46: Improved error message formatting consistencyChanged capitalization in error messages from "Failed" to "failed" for consistency in error message formatting.
Also applies to: 51-51, 70-70
pkg/blockchain/registryAdmin_test.go (2)
44-46: Properly handling new AddNode return valueThe test has been updated to correctly capture and validate the node ID that's now returned by the
AddNodemethod. The assertion that the node ID is not zero is appropriate for a successful node addition.
58-58: Correctly ignoring node ID in error test casesThe error test cases have been properly updated to use the blank identifier (
_) for the first return value, since these tests only care about verifying error conditions and not the returned node ID.Also applies to: 79-79
pkg/registry/contractRegistry.go (3)
213-226: Code correctly updated to use GetActiveNodesThe function has been updated to use
GetActiveNodesinstead ofAllNodes, which aligns with the PR objective of retrieving an array ofNodeWithIDrepresenting active nodes.
250-257: Node struct correctly updated with new fieldsThe Node struct has been properly updated to include
IsActiveandMinMonthlyFeefields, which matches the new contract structure.
232-248:❓ Verification inconclusive
Verify that IsActive correctly represents both API and replication status
According to the PR objectives, a node must have both API and replication enabled to be considered active. The code only checks
rawNode.Node.IsActivewithout explicitly verifying both conditions.
🏁 Script executed:
#!/bin/bash # Let's examine how IsActive is determined in the Nodes contract ast-grep --pattern 'function updateActive' | grep -A 20 'updateActive'Length of output: 70
I'll run a more targeted search within the repository to see if there's any function or logic that explicitly combines API and replication statuses. This will help clarify if
IsActiveindeed reflects both aspects as per review expectations.
🏁 Script executed:
#!/bin/bash # Search for any function named updateActive or similar, which might determine IsActive status rg "func updateActive" -n . # Also, search for any reference to replication along with IsActive rg "IsActive" -n .Length of output: 49422
Action Required: Verify Aggregation of Node Active Status
Please confirm that the node’s "IsActive" flag truly represents both API and replication enablement as required by the PR objectives. The current conversion function in
pkg/registry/contractRegistry.go(lines 232–248) only checksrawNode.Node.IsActive, without explicitly verifying that bothisApiEnabledandisReplicationEnabledare true. If "IsActive" is meant to be a computed aggregation of these two flags, ensure that its derivation is clear and robust. Otherwise, please adjust the logic to explicitly validate both conditions.pkg/testutils/config.go (2)
54-70: Function refactored to handle XMTPNodeRegistry.jsonThe function has been properly updated to handle the new contract naming convention and structure, now including support for extracting addresses from
XMTPNodeRegistry.json.
77-88: Contract configuration paths correctly updatedThe paths and function calls have been appropriately updated to use
getDeploymentAddressand reference the newXMTPNodeRegistry.jsonfile.pkg/blockchain/interface.go (1)
42-62: Interface properly extended with node management functionsThe NodeRegistry interface has been correctly expanded to include:
- Updated return type for AddNode (returning the nodeId)
- New methods for managing node properties:
- UpdateIsApiEnabled
- UpdateIsReplicationEnabled
- UpdateActive
These changes align perfectly with the PR objective of integrating the new Nodes contract with enhanced node management capabilities.
pkg/registry/contractRegistry_test.go (3)
6-6: Import ofmath/bigis appropriate for big integer usage.
No concerns here; this aligns with the new requirement of handling Node IDs as*big.Int.
48-59: Mocking newGetActiveNodescall is valid.
The mocked return values correctly provide two active nodes withbig.NewIntIDs. This setup is coherent with the updated interface.
92-106: Dynamic mocking enhances test coverage for changed node states.
The approach of returning different addresses after the initial call ensures that changes in node data are properly tested. This is a good way to verify the registry’s logic upon consecutive fetches.cmd/cli/main.go (9)
23-33: New CLI fields for node state updates.
These additions (e.g.,UpdateActive,UpdateApiEnabled,UpdateReplicationEnabled) align well with the expanded functionality for managing nodes.
48-54: Added option structs forupdate-active,update-api-enabled, andupdate-replication-enabled.
The corresponding commands are properly declared, matching each new option.
58-59: Error handling on command addition is consistent.
Returning explicit errors clarifies potential setup issues for these new subcommands.Also applies to: 61-62, 63-65, 66-68, 69-70
89-90: Graceful error handling for missing or invalid commands.
These checks ensure that users see a clear message when no command is specified or parsing fails.Also applies to: 95-95
104-104: Updated CLI initialization to include new fields.
This is a straightforward addition ensuring each new subcommand’s options are recognized.
157-172: New return value capturesnodeIdfromAddNode.
Logging the returnednodeIdprovides improved visibility into registered nodes. Ensure you handle this value carefully if additional actions or validations on the node ID are required.
175-213:updateActivefunction is consistent with the updated contract call.
It properly creates a new chain client, signer, and callsUpdateActive. This matches the PR objective of updating the active status.
253-292: Replication enablement command follows established pattern.
No issues noted. The approach is consistent with other update commands.
307-333:getAllNodesretrieval is consistent.
This function aligns with the newly introducedGetAllNodescontract method.pkg/blockchain/registryAdmin.go (3)
31-32: Interface conformance check is a good practice.
The_ NodeRegistry = &NodeRegistryAdmin{}ensures compile-time enforcement of the interface contract, preventing accidental drift.
136-164: Enabling the API for nodes.
The approach here parallelsUpdateActive. No immediate concerns, though consider the same confirmation time caveat.
166-195:UpdateIsReplicationEnabledlogic is consistent with the other update methods.
The overall pattern is consistent, with each step verifying a signer, submitting a transaction, and waiting for confirmation.pkg/mocks/registry/mock_NodesContract.go (5)
25-53: Method renamed fromAllNodestoGetActiveNodeswith updated return typeThis change aligns with the PR objective of updating
getActiveNodes()to return an array ofNodeWithID. The return type has been changed from[]nodes.NodesNodeWithIdto[]nodes.INodesNodeWithId, indicating a shift to interface-based types.
55-81: Updated expecter methods forGetActiveNodesThe helper methods for mocking
GetActiveNodeshave been properly updated to match the new method signature and return types.
83-111: Added new methodGetAllNodesA new method has been added to retrieve all nodes, complementing the
GetActiveNodesmethod that now has more specific active node criteria (requiring both API and replication to be enabled).
113-139: Added expecter methods forGetAllNodesThe mock expecter methods for
GetAllNodeshave been properly implemented with the correct return type of[]nodes.INodesNodeWithId.
25-139:❓ Verification inconclusive
Verify all callers are updated to use the new method names
These interface changes (renaming
AllNodestoGetActiveNodesand addingGetAllNodes) will require updates to all call sites. Additionally, ensure that any code handling the returned node objects is updated to work with theINodesNodeWithIdinterface instead of the concrete type.
🏁 Script executed:
#!/bin/bash # Search for any remaining usages of the old AllNodes method echo "Checking for remaining references to AllNodes method:" rg "AllNodes\(" --type go -A 2 -B 2 # Check for usages of the new methods echo -e "\nChecking for usages of GetActiveNodes:" rg "GetActiveNodes\(" --type go -A 2 -B 2 echo -e "\nChecking for usages of GetAllNodes:" rg "GetAllNodes\(" --type go -A 2 -B 2 # Check for references to the old return type echo -e "\nChecking for references to old NodesNodeWithId type:" rg "NodesNodeWithId" --type go -A 2 -B 2 # Check for references to the new interface type echo -e "\nChecking for references to new INodesNodeWithId interface:" rg "INodesNodeWithId" --type go -A 2 -B 2Length of output: 40901
Action Required: Verify and Update Caller Method Usage for Node Retrieval
Based on the verification:
Review Caller Updates:
Several call sites continue to useGetAllNodes—for example, incmd/cli/main.go(the helper functiongetAllNodes) and inpkg/blockchain/registryAdmin.go. Confirm that these callers are intended to invoke the new behavior for active nodes. If these were meant to be updated from the old “AllNodes” to the newGetActiveNodes, please update these call sites accordingly.Interface Consistency:
The interface now declares bothGetActiveNodesandGetAllNodes. Please ensure that each call site is using the appropriate method based on whether only active nodes or all nodes (including inactive ones) are required.Type Handling:
Verify that downstream code processing returned node objects is consistently working with theINodesNodeWithIdinterface rather than an outdated concrete type. The grep output shows that the new interface is in use, so double-check any conversions (e.g., in ABI conversion routines) to prevent subtle mismatches.Please verify these points to ensure that the method renaming and additions are applied consistently across the codebase.
| n.IsActive == other.IsActive && | ||
| n.MinMonthlyFee.Cmp(other.MinMonthlyFee) == 0 && |
There was a problem hiding this comment.
Potential nil pointer dereference in Equals method
The comparison of MinMonthlyFee fields does not check for nil values before calling Cmp(). This could cause a panic if either node has a nil MinMonthlyFee.
Add a nil check before comparing:
- 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.IsActive == other.IsActive && | |
| n.MinMonthlyFee.Cmp(other.MinMonthlyFee) == 0 && | |
| n.IsActive == other.IsActive && | |
| ((n.MinMonthlyFee == nil && other.MinMonthlyFee == nil) || | |
| (n.MinMonthlyFee != nil && other.MinMonthlyFee != nil && | |
| n.MinMonthlyFee.Cmp(other.MinMonthlyFee) == 0)) && |
There was a problem hiding this comment.
Nodes are created via a smart-contract, which has a parameter uint256 minMonthlyFee.
MinMonthlyFee should never be nil.
There was a problem hiding this comment.
🧩 Analysis chain
@fbac Thank you for the clarification. You're right - since nodes are created via a smart contract with a uint256 minMonthlyFee parameter, and Solidity's uint256 type cannot be nil, the MinMonthlyFee field should always be properly initialized with at least a zero value.
I'll make note of this architectural guarantee for future reviews.
🏁 Script executed:
#!/bin/bash
# Find where Node structs are created or where MinMonthlyFee is set
echo "Searching for Node struct initialization..."
rg "Node\s*{" --type go
echo "Searching for MinMonthlyFee initialization..."
rg "MinMonthlyFee\s*:" --type go
echo "Searching for convertNode function to see how nodes are created from contract data..."
rg "func convertNode" --type go -A 20
Length of output: 1993
🏁 Script executed:
#!/bin/bash
# Let's examine the smart contract implementation
echo "Looking at the Nodes.sol contract to verify minMonthlyFee handling:"
rg -A 5 -B 5 "minMonthlyFee" contracts/src/Nodes.sol
# Let's also check if there are any tests that might help us understand the behavior
echo "Checking tests related to MinMonthlyFee:"
rg "MinMonthlyFee" --type go contracts/test
rg "MinMonthlyFee" --type go pkg/registry
Length of output: 2075
@fbac Thank you for the clarification. You're right - since nodes are created via a smart contract with a uint256 minMonthlyFee parameter, and Solidity's uint256 type cannot be nil, the MinMonthlyFee field should always be properly initialized.
Looking at the contract code, I can see the value is explicitly set during node creation:
_nodes[nodeId] = Node(signingKeyPub, httpAddress, false, false, false, minMonthlyFee);And in the Go code, it's directly mapped from the contract data.
✏️ Learnings added
Learnt from: fbac
PR: xmtp/xmtpd#550
File: pkg/registry/node.go:36-37
Timestamp: 2025-02-25T16:17:35.853Z
Learning: In the Node struct, MinMonthlyFee (*big.Int) is guaranteed to never be nil because nodes are created via a smart contract with a uint256 minMonthlyFee parameter, which cannot be nil in Solidity.
Learnt from: fbac
PR: xmtp/xmtpd#550
File: pkg/registry/node.go:36-37
Timestamp: 2025-02-25T16:17:35.853Z
Learning: In the Node struct, MinMonthlyFee (*big.Int) is guaranteed to never be nil because nodes are created via a smart contract with a uint256 minMonthlyFee parameter, which cannot be nil in Solidity.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/testutils/config.go (1)
47-66: Function refactoring looks good, but consider enhancing error handling.The function
getDeploymentAddressis a good refactoring that consolidates the contract address retrieval logic. The addition of the case forXMTPNodeRegistry.jsoncorrectly supports the new Nodes contract integration.Consider enhancing the error handling to provide more useful feedback when JSON fields are missing. The current implementation will return an empty string in the default case, which could lead to subtle bugs. A more robust approach might be:
func getDeploymentAddress(t *testing.T, fileName string) string { data, err := os.ReadFile(fileName) if err != nil { t.Fatalf("Failed to read json: %v", err) } + var address string switch { case strings.Contains(fileName, "GroupMessages.json"): - return fastjson.GetString(data, "addresses", "groupMessagesProxy") + address = fastjson.GetString(data, "addresses", "groupMessagesProxy") case strings.Contains(fileName, "IdentityUpdates.json"): - return fastjson.GetString(data, "addresses", "identityUpdatesProxy") + address = fastjson.GetString(data, "addresses", "identityUpdatesProxy") case strings.Contains(fileName, "XMTPNodeRegistry.json"): - return fastjson.GetString(data, "addresses", "XMTPNodeRegistry") + address = fastjson.GetString(data, "addresses", "XMTPNodeRegistry") default: - return "" + t.Fatalf("Unrecognized contract JSON file: %s", fileName) + return "" } + if address == "" { + t.Fatalf("Failed to extract address from %s", fileName) + } + return address }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dev/docker/Dockerfile(1 hunks)dev/docker/Dockerfile-cli(1 hunks)pkg/testutils/config.go(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- dev/docker/Dockerfile
- dev/docker/Dockerfile-cli
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test (Node)
🔇 Additional comments (1)
pkg/testutils/config.go (1)
73-84: Contract address retrieval updated correctly.The updates to use
getDeploymentAddressfor all contract addresses are consistent with the function refactoring. The path change to useXMTPNodeRegistry.jsonaligns with the PR objective of integrating the new Nodes contract.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/cli/main.go (1)
310-315:⚠️ Potential issueLogging private keys is a significant security risk.
Exposing the private key in plaintext logs can lead to unintended credential leaks. Consider removing or masking this output.
-logger.Info( - "generated private key", - zap.String("private-key", utils.EcdsaPrivateKeyToString(privKey)), - zap.String("public-key", utils.EcdsaPublicKeyToString(privKey.Public().(*ecdsa.PublicKey))), - zap.String("address", utils.EcdsaPublicKeyToAddress(privKey.Public().(*ecdsa.PublicKey))), -) +logger.Info( + "generated private key", + zap.String("private-key", "[REDACTED]"), + zap.String("public-key", utils.EcdsaPublicKeyToString(privKey.Public().(*ecdsa.PublicKey))), + zap.String("address", utils.EcdsaPublicKeyToAddress(privKey.Public().(*ecdsa.PublicKey))), +)
🧹 Nitpick comments (3)
dev/register-local-node (1)
8-8: Security enhancement: Consider using a dedicated operator key.The script now uses
ANVIL_ACC_1_PRIVATE_KEYfor both the node address and operator private key. For better security separation, consider using a different account for the node operator role.pkg/blockchain/registryAdmin.go (2)
110-200: Consider adding parameter validation in new methods.While
AddNodevalidates parameters, the new methods (UpdateActive,UpdateIsApiEnabled,UpdateIsReplicationEnabled) don't validate nodeId. Consider adding validation to ensure nodeId is valid before making contract calls.func (n *NodeRegistryAdmin) UpdateActive( ctx context.Context, nodeId uint32, isActive bool, ) error { + if nodeId == 0 { + return fmt.Errorf("invalid nodeId: %d", nodeId) + } if n.signer == nil { return fmt.Errorf("no signer provided") }
110-200: Consider configurable transaction wait timeouts.All transaction methods use hardcoded values (2 seconds, 250 milliseconds) for transaction confirmation. Consider making these configurable, especially for different network conditions.
You could add these parameters to the
NodeRegistryAdminstruct and set them during initialization:type NodeRegistryAdmin struct { client *ethclient.Client signer TransactionSigner contract *nodes.Nodes logger *zap.Logger + txTimeout time.Duration + txPollInterval time.Duration } func NewNodeRegistryAdmin(...) { // ... return &NodeRegistryAdmin{ // ... + txTimeout: 2 * time.Second, + txPollInterval: 250 * time.Millisecond, }, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/cli/main.go(9 hunks)dev/register-local-node(1 hunks)dev/register-local-node-2(1 hunks)pkg/blockchain/interface.go(2 hunks)pkg/blockchain/registryAdmin.go(3 hunks)pkg/blockchain/registryAdmin_test.go(3 hunks)pkg/config/cliOptions.go(1 hunks)pkg/mocks/blockchain/mock_NodeRegistry.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/blockchain/interface.go
- dev/register-local-node-2
- pkg/config/cliOptions.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Test (Node)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (30)
dev/register-local-node (4)
10-15: Successfully integrated the newmin-monthly-feeparameter.The updated registration command correctly implements the new contract requirements.
17-18: Good approach to extract the node ID.Using jq to parse the JSON response is a reliable method for extracting the node ID.
20-32: Correctly implemented node activation sequence.This implementation aligns with the PR's objective of requiring both API and replication to be enabled for a node to be active. The script now performs a complete node setup sequence:
- Updates API enabled status
- Updates replication enabled status
- Activates the node
The command parameter ordering and structure are consistent.
34-34: Clear success message with node ID indication.Good addition of a color-highlighted success message that includes the node ID.
cmd/cli/main.go (8)
32-33: Correctly added new CLI struct fields for node state management.The CLI struct properly extends to support the new node state management commands.
54-55: Added options structs for the new commands.The declared options variables align with the new commands being added to the CLI.
64-72: Well-structured command definitions with clear error handling.The new CLI commands have appropriate descriptions and error handling. The three new commands align with the PR objectives for managing node states.
158-165: Effective validation for the new minimum monthly fee parameter.The implementation correctly:
- Parses the minimum monthly fee from string to big.Int
- Validates that parsing was successful
- Ensures the value is not negative
This is an important safeguard for handling blockchain value parameters.
186-224: Implemented the updateActive function with proper authorization.This function correctly:
- Creates a blockchain client with appropriate context
- Initializes a signer with the admin's private key
- Sends the update request with the node ID and active status
The implementation aligns with the PR objective of updating the active status of nodes.
226-263: Implemented updateApiEnabled function with operator authorization.This function properly uses the operator's private key for authorization, which is a different authorization model than the other update functions that use admin authorization.
265-303: Implemented updateReplicationEnabled with admin authorization.The function correctly uses admin authorization and passes both the node ID and the enabled flag.
412-420: Added control flow for the new commands.The updated main function properly handles the new commands by dispatching to the appropriate handlers.
pkg/blockchain/registryAdmin_test.go (9)
14-14: Enhanced test helper function to return the signer.Updated
buildRegistryto return the signer, which enables more comprehensive testing of authorization scenarios.Also applies to: 33-33
38-58: Good test helper for node creation with randomized inputs.The new
addRandomNodehelper function:
- Creates randomized test data
- Calls the node registration method
- Verifies the node ID is non-zero
- Returns necessary values for further testing
This improves test readability and reduces code duplication.
60-65: Simplified test using the new helper function.The test now uses the helper function, making it more concise while still covering the functionality.
80-96: Added test for negative minimum monthly fee validation.Good test case that verifies the registry correctly rejects negative values for the minimum monthly fee.
125-144: Added test for UpdateIsApiEnabled with node owner authorization.This test verifies:
- The node owner can update the API enabled status
- The function correctly handles successful responses
The test setup correctly uses the registry signer as the node owner to test this authorization path.
146-157: Added test for unauthorized UpdateIsApiEnabled calls.Good test that verifies the API rejects updates from non-owner accounts with the appropriate error message.
159-165: Added test for invalid node IDs in UpdateIsApiEnabled.The test verifies the API returns an authorization error when attempting to update a non-existent node.
167-176: Added test for UpdateIsReplicationEnabled.The test verifies that an admin can successfully update the replication enabled status.
178-201: Added comprehensive test for the node activation sequence.This test validates the full node activation flow required by the new contract:
- Register a node
- Enable API
- Enable replication
- Activate the node
This test ensures the entire sequence works as expected, which is crucial to the PR objectives.
pkg/mocks/blockchain/mock_NodeRegistry.go (1)
1-244: Mock NodeRegistry implementation properly matches the contract interface.The auto-generated mock correctly implements:
- AddNode with the new minMonthlyFee parameter
- UpdateActive for activating/deactivating nodes
- UpdateIsApiEnabled for toggling API availability
- UpdateIsReplicationEnabled for toggling replication
The mock functions have appropriate parameter types and return values.
pkg/blockchain/registryAdmin.go (8)
31-32: Good addition of interface compliance check.This explicit check ensures that
NodeRegistryAdminproperly implements theNodeRegistryinterface at compile time, which is a best practice for type safety.
60-61: The signature change aligns with the PR objectives.Adding the
minMonthlyFeeparameter and returning thenodeIdenhances the API by allowing fee specification and enabling tracking of the newly created node.
66-68: Good validation of the new parameter.The validation ensures
minMonthlyFeeis neither nil nor negative, preventing invalid values from reaching the contract.
80-107: Well-implemented node addition with event processing.The implementation properly passes the new parameter to the contract and extracts the nodeId from the transaction logs, which is the correct approach for retrieving contract-generated IDs.
110-139: New UpdateActive method implements required PR functionality.This method allows setting a node's active status directly, which is consistent with the PR objective of updating the active status based on specific conditions.
171-200: UpdateIsReplicationEnabled method implements required PR functionality.This method allows setting a node's replication status, which directly supports the PR objective of making node activity dependent on both API and replication being enabled.
235-235: Return type change from concrete type to interface.Changed from
[]nodes.NodesNodeWithIdto[]nodes.INodesNodeWithId, indicating a move toward interface-based programming which improves flexibility.
141-169:❓ Verification inconclusive
UpdateIsApiEnabled lacks a boolean parameter unlike other methods.
The function doesn't have a parameter to specify whether API should be enabled or disabled, unlike similar methods like
UpdateActiveandUpdateIsReplicationEnabled. This inconsistency suggests the contract method toggles the current state rather than setting it explicitly.
🏁 Script executed:
#!/bin/bash # Check the signature of the UpdateIsApiEnabled method in the contract rg -A 5 "function UpdateIsApiEnabled" --glob="*.sol"Length of output: 54
Action Required: Verify the Contract’s Intended API Behavior
The current Go implementation of
UpdateIsApiEnabledcalls a contract method with only a node ID, while similar functions likeUpdateActiveandUpdateIsReplicationEnabledinclude an explicit boolean parameter. However, our search in the Solidity source did not return a signature forUpdateIsApiEnabled, so it’s unclear whether this was an intentional design (i.e., a toggle mechanism) or an inconsistency.
- Inconsistency Noted: Unlike
UpdateActiveandUpdateIsReplicationEnabled, no boolean parameter is provided.- Verification Needed: Manually confirm that the underlying contract’s
UpdateIsApiEnabledfunction is meant to toggle API state without accepting a boolean. If this is unintentional, update the function signature accordingly.
fbac
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/cli/main.go (1)
309-320:⚠️ Potential issueSecurity risk: Private key is logged in plaintext.
Exposing the private key in logs is a significant security risk as it can lead to credential leaks.
-logger.Info( - "generated private key", - zap.String("private-key", utils.EcdsaPrivateKeyToString(privKey)), - zap.String("public-key", utils.EcdsaPublicKeyToString(privKey.Public().(*ecdsa.PublicKey))), - zap.String("address", utils.EcdsaPublicKeyToAddress(privKey.Public().(*ecdsa.PublicKey))), -) +logger.Info( + "generated private key", + zap.String("private-key", "[REDACTED]"), + zap.String("public-key", utils.EcdsaPublicKeyToString(privKey.Public().(*ecdsa.PublicKey))), + zap.String("address", utils.EcdsaPublicKeyToAddress(privKey.Public().(*ecdsa.PublicKey))), +)
🧹 Nitpick comments (7)
cmd/cli/main.go (1)
186-225: Consider extracting common client and registry setup code.The client creation, signer creation, and registry admin setup are duplicated across multiple command handlers.
Consider extracting a helper function to reduce duplication:
+func createRegistryAdmin(logger *zap.Logger, contractsOptions config.ContractsOptions, privateKey string) (*blockchain.NodeRegistryAdmin, error) { + ctx := context.Background() + chainClient, err := blockchain.NewClient(ctx, contractsOptions.RpcUrl) + if err != nil { + return nil, fmt.Errorf("could not create chain client: %w", err) + } + + signer, err := blockchain.NewPrivateKeySigner( + privateKey, + contractsOptions.ChainID, + ) + if err != nil { + return nil, fmt.Errorf("could not create signer: %w", err) + } + + registryAdmin, err := blockchain.NewNodeRegistryAdmin( + logger, + chainClient, + signer, + contractsOptions, + ) + if err != nil { + return nil, fmt.Errorf("could not create registry admin: %w", err) + } + + return registryAdmin, nil +}This would simplify each command handler significantly.
pkg/blockchain/registryAdmin.go (4)
66-67: Fee validation
You are convertingminMonthlyFeeto a string using%sin the error. Depending on Go’s formatting, that might produce an unexpected pointer output. Consider usingminMonthlyFee.String()for a clearer error message:- return 0, fmt.Errorf("invalid min monthly fee provided %s", minMonthlyFee) + return 0, fmt.Errorf("invalid min monthly fee provided %s", minMonthlyFee.String())
110-139: UpdateActive method
This method follows a consistent pattern: check signer, call contract, and wait for confirmation. If repeated throughout, consider extracting the waiting logic into a helper to reduce code duplication.
141-170: UpdateIsApiEnabled method
As withUpdateActive, the pattern is duplicated. A shared utility for transaction creation and confirmation may help maintain DRY principles if you foresee more methods of this nature.
172-201: UpdateIsReplicationEnabled method
This method mirrors the approach inUpdateActiveandUpdateIsApiEnabled. If you keep adding similar updates, consider a centralized function to handle contract calls and wait logic.contracts/src/Nodes.sol (2)
5-7: Role-based imports
UsingAccessControlDefaultAdminRulesandEnumerableSetoffers granular role management and efficient set operations. Watch out for potential extra gas usage in large sets or frequent enumerations.
90-103: transferFrom override
Deactivating the node after transfer enforces that the new owner must explicitly requalify the node as active, aligning with the design that an active node must meet certain criteria.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/cli/main.go(9 hunks)contracts/src/Nodes.sol(1 hunks)contracts/src/interfaces/INodes.sol(4 hunks)contracts/test/Nodes.sol(14 hunks)dev/register-local-node(1 hunks)dev/register-local-node-2(1 hunks)pkg/blockchain/interface.go(2 hunks)pkg/blockchain/registryAdmin.go(3 hunks)pkg/blockchain/registryAdmin_test.go(3 hunks)pkg/config/cliOptions.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- dev/register-local-node-2
- dev/register-local-node
- pkg/blockchain/interface.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Test (Node)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (60)
pkg/config/cliOptions.go (4)
32-33: LGTM: Good addition of required minimum monthly fee.The addition of the
MinMonthlyFeefield to theRegisterNodeOptionsis well-structured with appropriate documentation and correctly marked as required.
35-39: LGTM: Well-structured option type for updating node active status.The
UpdateActiveOptionsstruct follows the established pattern with clear field descriptions and appropriate types.
41-45: LGTM: Good implementation of API enablement options.The
UpdateApiEnabledOptionsstruct correctly usesOperatorPrivateKeyinstead ofAdminPrivateKey, reflecting that operators (not just admins) can control API enablement.
47-51: LGTM: Well-defined replication enablement options.The
UpdateReplicationEnabledOptionsstruct properly requires the admin's private key for authentication, maintaining security controls.pkg/blockchain/registryAdmin_test.go (7)
14-33: Good improvement to function signature.Returning the signer from
buildRegistrymakes tests more maintainable by avoiding duplicate signer creation code.
38-58: Well-structured helper function.The
addRandomNodefunction effectively encapsulates node creation logic, improving test readability and reducing duplication.
80-96: Good test case for negative minimum monthly fee.This test properly validates that the system rejects negative fees, an important validation for financial parameters.
125-144: Good test for UpdateIsApiEnabled.The test correctly simulates the owner updating API enablement, including valuable context comment that explains owner-only restrictions.
146-157: Comprehensive unauthorized access test.The test appropriately verifies that non-owners cannot update API enablement, and includes validation of the expected error message.
159-165: Good edge case testing.Testing with an invalid/non-existent node ID ensures robust error handling for the API.
178-201: Thorough test for the combined node activation flow.This test correctly validates the full activation sequence, ensuring both API and replication must be enabled before activating a node, matching the new requirements.
cmd/cli/main.go (2)
158-165: Good validation of minimum monthly fee.The code properly validates that the fee can be parsed and is non-negative, preventing invalid input.
416-424: LGTM: Command dispatch implemented correctly.The main function dispatch logic properly routes to the new command handlers.
contracts/test/Nodes.sol (5)
7-7: LGTM: Contract updated to use the new Nodes contract.Import correctly updated from NodesV2 to Nodes.
17-18: LGTM: Instance variable updated for the new contract.Declaration properly changed to use the new Nodes contract type.
29-30: LGTM: Constructor call updated for new contract.The instantiation is correctly updated to use the new Nodes contract constructor.
422-428: Good test for new activation requirements.This test correctly verifies that nodes cannot be activated without both API and replication being enabled, supporting the new contract requirements.
864-870: Well-structured helper function.The
_enableNodeshelper improves code readability and reduces duplication by encapsulating the common pattern of enabling both API and replication for multiple nodes.pkg/blockchain/registryAdmin.go (6)
31-31: Interface compliance check
This line confirms at compile-time thatNodeRegistryAdminimplements theNodeRegistryinterface. It’s a good practice to ensure the struct remains in sync with interface requirements.
60-64: Expanded AddNode signature
IntroducingminMonthlyFeeand returning(nodeId, err)enhances clarity by allowing the function to convey both the newly assigned node ID and any error. The validation of theowneraddress is a solid step; however, consider including a more descriptive error message or additional checks (e.g., zero address) if needed.
73-74: Signer check
Returning an error ifn.signeris nil helps prevent erroneous transactions. This preflight check is well-placed.
80-84: Contract call handling
Wrapping the contract call with the provided context and checking for errors is good. This ensures any issues from the contract invocation are immediately surfaced.
86-108: Event parsing logic
Waiting for the transaction receipt and iterating over logs to find theNodeAddedevent is a reliable way to finalize the node creation. The loop stops at the first successful parse, which is likely correct for a single node addition. If multiple node additions in one TX are ever possible, you might need to handle them collectively.
236-236: Return type changed
TheGetAllNodesmethod now returns a slice of theINodesNodeWithIdinterface rather than a concrete type. Ensure all consumers can handle this interface-based result and do not rely on the old struct.contracts/src/interfaces/INodes.sol (4)
19-50: New error definitions
These new specialized error types help clarify specific failure conditions like invalid fees, URIs, or node config. More granular errors improve debugging and user feedback.
223-224: Updated function signature for API toggle
Explicitly passingbool isApiEnabledmakes this function more transparent. Verify that all callers are updated to pass the correct on/off flag.
235-236: Changed return array
getAllNodesnow returnsNodeWithId[], ensuring each node includes its ID. This is likely a breaking change for code expecting the older structure, so update dependent code accordingly.
250-250: Return type for active nodes
getActiveNodesnow returns aNodeWithId[], streamlining the interface across node-retrieval methods. Update client logic if it previously expected only node details without IDs.contracts/src/Nodes.sol (32)
10-22: Enhanced documentation
These updated doc comments provide a clear overview of the contract’s node-handling logic and role-based responsibilities, which aids both internal developers and external integrators.
23-23: Transition to role-based security
ReplacingOwnablewithAccessControlDefaultAdminRulesprovides more flexible privilege assignment. Validate that any external scripts or integrations relying onOwnableare appropriately updated.
25-25: EnumerableSet usage
EnumerableSet.UintSetis a fitting choice for quick membership checks and enumerations of active nodes.
31-31: BPS constant
Defining 10,000 asMAX_BPSis a well-established pattern for basis points, ensuring 100% coverage and avoiding fractional overflows.
35-35: Private _baseTokenURI
Storing the base URI in a private variable is standard. The_baseURI()override references it, keeping the metadata path flexible while restricting direct writes.
37-37: Default cap on active nodes
maxActiveNodesis set to 20, with the ability to update viaupdateMaxActiveNodes. Ensure this default aligns with real-world capacity requirements.
44-44: Node counter
Initializing_nodeCounterto zero is straightforward, especially since node IDs grow in steps of 100. No issues noted.
46-46: _nodes mapping
Private storage of node data ensures proper encapsulation, with external queries routed through dedicated getters.
49-49: _activeNodes set
Using anEnumerableSetfor tracking active node IDs improves synchronization between active/inactive states without extra overhead.
52-52: nodeOperatorCommissionPercent
Storing commission in basis points and capping it viaMAX_BPSprevents impossible fee scenarios (>100%).
59-69: Constructor logic
GrantingADMIN_ROLEandNODE_MANAGER_ROLEto_initialAdmin, while requiring a non-zero admin address, is a solid foundation for role-based governance.
71-88: addNode method
Input validation for addresses, signing keys, and HTTP endpoints is robust. The newly introducedminMonthlyFeeparameter is stored without bounding checks, so confirm whether negative or overly large values should be disallowed at the higher levels.
105-111: updateHttpAddress
Only theNODE_MANAGER_ROLEcan update a node’s HTTP address, centralizing endpoint modifications. If node operators should self-manage addresses, consider delegating a more granular role or approach.
113-118: updateMinMonthlyFee
Similar restricted approach, requiringNODE_MANAGER_ROLE. This ensures consistent fee updates from authorized users or managers.
120-130: updateIsApiEnabled
Allowing only the node’s owner to toggle API status is reasonable. The auto-deactivation if disabled enforces the condition that active nodes must have API enabled.
132-142: updateIsReplicationEnabled
Restricted toNODE_MANAGER_ROLE. Automatically deactivating nodes lacking replication is consistent with the revised rule that nodes must have both replication and API enabled to be active.
144-161: updateActive
Requiring a node to have both replication and API enabled before marking it active is fully aligned with the updated requirements. The checks for existing active/inactive states and maxActiveNodes help maintain consistency.
163-172: batchUpdateActive
Looping through each node is simple and explicit. For very large batches, be mindful of gas costs, but for administrative usage, it’s probably sufficient.
174-179: updateMaxActiveNodes
EnsuringnewMaxActiveNodesis greater than the current active node count prevents forcibly deactivating nodes. This maintains a sensible progression in system limits.
181-186: updateNodeOperatorCommissionPercent
Restricting the new commission to<= MAX_BPSkeeps rates within 0-100%. This is a solid boundary.
188-194: setBaseURI
Requiring a trailing slash innewBaseURIis good practice for NFT metadata. This avoids edge cases in token URI concatenation.
196-206: getAllNodes
Allocating an array of size_nodeCountercan yield empty slots for burned or never-minted IDs. If that’s acceptable, this method is fine. Otherwise, you might want to filter those out to avoid misleading data.
208-211: getAllNodesCount
This simply reflects_nodeCounter, so it includes every minted node ID, even if burned. Consumers should be aware.
213-217: getNode
Requiring that the node exists with_nodeExists(nodeId)is a sensible safeguard.
219-229: getActiveNodes
Collecting only IDs in_activeNodesintoNodeWithId[]is straightforward and ensures consistent results. Any missing or deactivated node ID is skipped.
231-234: getActiveNodesIDs
Returning_activeNodes.values()offers a simple read of active node identifiers.
236-239: getActiveNodesCount
Relying on_activeNodes.length()is efficient and consistent with the set-based storage of active nodes.
241-244: getNodeIsActive
This boolean check is a convenient shortcut for external callers needing to confirm a node’s active status.
246-251: _nodeExists helper
Returning_ownerOf(nodeId) != address(0)is a quick and direct approach to detect minted and unburned nodes.
253-256: _baseURI override
The contract’s_baseURIoverride ties in neatly with your stored_baseTokenURI.
258-266: _deactivateNode
Removing the node from_activeNodesand updating itsisActivestatus in one place reduces duplication and ensures consistent event emission.
268-276: supportsInterface override
Combining the parent contract interfaces in a single override is the correct approach for multi-inheritance compliance with ERC165 checks.
TL;DR
Integrate the new Nodes contract into
xmptdcode.What changed
getActiveNodes()now returns an array ofNodeWithID.updateActive()requires the node to have api and replication enabled.Closes #541
Closes #542
Closes #543
Summary by CodeRabbit
Summary by CodeRabbit
New Features
UpdateActive,UpdateIsApiEnabled, andUpdateIsReplicationEnabled.Refactor
Chores