Migrate RateRegistry and NodeRegistry contracts to 0.4.0 version with parameter registry integration and updated data types #810
Conversation
WalkthroughThis change removes all CLI and backend logic related to node fees, rate management, and commission percentages, replacing them with functionality for managing the maximum canonical node limit. It updates contract interactions and types to match a new NodeRegistry contract, removes rate and fee-related methods and tests, and adapts supporting code and environment variables accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant NodeRegistryAdmin
participant NodeRegistryCaller
User->>CLI: set-max-canonical (limit)
CLI->>NodeRegistryAdmin: SetMaxCanonical(limit)
NodeRegistryAdmin-->>CLI: Success/Error
CLI-->>User: Output result
User->>CLI: get-max-canonical-nodes
CLI->>NodeRegistryCaller: GetMaxCanonicalNodes()
NodeRegistryCaller-->>CLI: limit value
CLI-->>User: Output limit
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.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
pkg/config/cliOptions.go (1)
18-18:⚠️ Potential issueInconsistent NodeId type in NodeRegistryManagerOptions.
While all other NodeId fields in the file have been changed from
int64touint32, this one remainsint64. This inconsistency could cause type conversion issues when interacting with the updated interfaces.Update the NodeId type to match other NodeId fields:
- NodeId int64 `long:"node-id" description:"NodeId of the node to administer"` + NodeId uint32 `long:"node-id" description:"NodeId of the node to administer"`pkg/fees/contractRates.go (1)
134-159: 🛠️ Refactor suggestionConcurrent read/write to
c.ratesis unsynchronised
refreshLoopmutatesc.rateson a background goroutine whileGetRatesreads it without any locking. A reader could see a partially-written slice, leading to panics or wrong data.Add an
RWMutex, e.g.:type ContractRatesFetcher struct { ... mu sync.RWMutex }Guard
c.ratesandc.lastRefreshaccesses with the mutex.
🧹 Nitpick comments (6)
pkg/blockchain/migrator/migrator.go (1)
45-45: Field mapping between different naming conventions.The code now maps
node.Node.IsCanonicaltoInCanonicalNetworkin the serializable struct. This line bridges the gap between the two different naming conventions but might make the code harder to maintain.Consider aligning the field names across all related structs for better consistency:
- InCanonicalNetwork: node.Node.IsCanonical, + IsCanonical: node.Node.IsCanonical,And update the struct definition accordingly.
dev/local.env (1)
29-34: Consider adding deprecation timelineThe single chain deployment variables are marked as deprecated, but no timeline for removal is specified. Adding a removal timeline would help with future maintenance planning.
-# Deprecated! single chain deployments +# Deprecated! single chain deployments (to be removed in v0.5.0)pkg/blockchain/registryCaller.go (1)
64-71: Consider optimizing type conversionThe conversion from
uint32toint64and then tobig.Intin theOwnerOfmethod could be simplified by converting directly tobig.Int.- }, big.NewInt(int64(nodeId))) + }, new(big.Int).SetUint64(uint64(nodeId)))pkg/fees/contractRates.go (1)
46-46: Avoid hard-wiring to the concrete contract caller
contract *rateregistry.RateRegistryCallermakes unit-testing harder because the struct is no longer mockable through theRatesContractinterface declared above. Consider keeping the field typed as the interface or injecting a test double.- contract *rateregistry.RateRegistryCaller + contract RatesContractcmd/cli/main.go (2)
31-44: StaleAddRatesfield – dead code left behind
AddRatesoptions still exist in theCLIstruct, but the corresponding command has been removed from the parser and theswitch. Keeping unused fields adds cognitive load and may confuse users looking for the feature.If the command is gone for good, please delete the field and the related option declaration variables.
158-158: No-op call can be removed
privKey.Public()is invoked for its side-effects, but it returns the public key which is ignored.
The call is unnecessary and can be dropped.- privKey.Public()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
dev/gen/abiis excluded by!**/gen/**
📒 Files selected for processing (26)
cmd/cli/main.go(4 hunks)dev/docker/docker-compose-register.yml(4 hunks)dev/docker/docker-compose.yml(1 hunks)dev/local.env(2 hunks)pkg/blockchain/blockchainPublisher_test.go(0 hunks)pkg/blockchain/migrator/migrator.go(2 hunks)pkg/blockchain/migrator/migrator_test.go(1 hunks)pkg/blockchain/ratesAdmin.go(0 hunks)pkg/blockchain/ratesAdmin_test.go(0 hunks)pkg/blockchain/registryAdmin.go(8 hunks)pkg/blockchain/registryAdmin_test.go(2 hunks)pkg/blockchain/registryCaller.go(2 hunks)pkg/config/cliOptions.go(2 hunks)pkg/fees/contractRates.go(4 hunks)pkg/fees/contractRates_test.go(7 hunks)pkg/indexer/e2e_test.go(0 hunks)pkg/indexer/storer/groupMessage_test.go(0 hunks)pkg/indexer/storer/identityUpdate_test.go(0 hunks)pkg/mocks/fees/mock_RatesContract.go(3 hunks)pkg/registry/contractRegistry.go(2 hunks)pkg/registry/contractRegistry_test.go(3 hunks)pkg/registry/node.go(2 hunks)pkg/testutils/contractRegistry.go(1 hunks)pkg/testutils/contracts.go(1 hunks)pkg/testutils/contracts_test.go(0 hunks)pkg/testutils/registry/registry.go(1 hunks)
💤 Files with no reviewable changes (7)
- pkg/blockchain/ratesAdmin_test.go
- pkg/indexer/storer/identityUpdate_test.go
- pkg/blockchain/blockchainPublisher_test.go
- pkg/indexer/storer/groupMessage_test.go
- pkg/testutils/contracts_test.go
- pkg/indexer/e2e_test.go
- pkg/blockchain/ratesAdmin.go
🧰 Additional context used
🧠 Learnings (1)
pkg/registry/node.go (1)
Learnt from: fbac
PR: xmtp/xmtpd#550
File: pkg/registry/node.go:36-37
Timestamp: 2025-02-25T16:17:36.032Z
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.
🧬 Code Graph Analysis (10)
pkg/testutils/contractRegistry.go (1)
pkg/registry/node.go (1)
Node(15-21)
pkg/blockchain/migrator/migrator.go (2)
pkg/registry/node.go (1)
Node(15-21)pkg/utils/keys.go (1)
EcdsaPublicKeyToString(30-32)
pkg/blockchain/migrator/migrator_test.go (2)
pkg/blockchain/migrator/migrator.go (1)
SerializableNode(14-20)pkg/utils/keys.go (1)
EcdsaPublicKeyToString(30-32)
pkg/fees/contractRates_test.go (2)
pkg/testutils/log.go (1)
NewLog(17-25)pkg/abi/rateregistry/RateRegistry.go (1)
IRateRegistryRates(33-39)
pkg/registry/contractRegistry_test.go (2)
pkg/registry/node.go (1)
Node(15-21)pkg/abi/noderegistry/NodeRegistry.go (1)
INodeRegistryNode(33-38)
pkg/mocks/fees/mock_RatesContract.go (2)
pkg/fees/interface.go (1)
Rates(13-18)pkg/abi/rateregistry/RateRegistry.go (1)
IRateRegistryRates(33-39)
pkg/fees/contractRates.go (2)
pkg/fees/interface.go (1)
Rates(13-18)pkg/abi/rateregistry/RateRegistry.go (2)
IRateRegistryRates(33-39)RateRegistryCaller(80-82)
cmd/cli/main.go (2)
pkg/config/cliOptions.go (10)
RegisterNodeOptions(57-63)NetworkAdminOptions(65-68)GetAllNodesOptions(29-31)GetNodeOptions(33-35)SetHttpAddressOptions(70-74)MigrateNodesOptions(37-40)AddRatesOptions(46-53)GetRatesOptions(55-55)IdentityUpdatesStressOptions(76-82)WatcherOptions(84-87)pkg/stress/watcher.go (1)
Watcher(17-22)
pkg/registry/contractRegistry.go (1)
pkg/registry/node.go (1)
Node(15-21)
pkg/blockchain/registryCaller.go (1)
pkg/abi/noderegistry/NodeRegistry.go (1)
INodeRegistryNode(33-38)
🪛 Gitleaks (8.26.0)
pkg/testutils/contracts.go
17-17: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (45)
pkg/testutils/contractRegistry.go (2)
11-14: LGTM! Field name update is consistentThe renaming of
InCanonicalNetworktoIsCanonicalaligns with the updated field name in theregistry.Nodetype.
20-23: LGTM! Field name update is consistentThe renaming of
InCanonicalNetworktoIsCanonicalaligns with the updated field name in theregistry.Nodetype while maintaining the expected behavior for unhealthy nodes.pkg/registry/node.go (2)
16-20: LGTM! Node struct simplificationThe
Nodestruct has been simplified by removing theMinMonthlyFeeMicroDollarsfield and renamingInCanonicalNetworktoIsCanonical. This aligns with the objective of removing fee-related features from the codebase.
34-34: LGTM! Updated field comparison in Equals methodThe
Equalsmethod correctly updates the field comparison to use the renamedIsCanonicalfield.pkg/testutils/registry/registry.go (1)
14-18: LGTM! Test utility updated to match new Node structThe
CreateNodefunction has been updated to use the renamedIsCanonicalfield and remove theMinMonthlyFeeMicroDollarsfield, maintaining consistency with the changes to theNodestruct.pkg/blockchain/registryAdmin_test.go (2)
68-71: LGTM - Successfully removed minimum monthly fee parameterThe code successfully removes the
minMonthlyFeeMicroDollarsparameter from theAddNodecall, which aligns with the migration to the new contract version that no longer includes fee management functionality.
79-81: LGTM - Parameter list updated consistentlyThis test case has been updated consistently with the other test to reflect the removal of the fee parameter from the
AddNodemethod.dev/docker/docker-compose-register.yml (4)
4-6: LGTM - Using local build instead of fixed image tagSwitching to a local build configuration is a good change as it ensures that the latest code changes are always used during development and testing.
44-46: LGTM - Consistent build configurationGood, using the same build configuration across all services ensures consistency.
66-68: LGTM - Consistent build configurationBuild configuration is properly applied to all services in the compose file.
23-26:⚠️ Potential issueRemove duplicate image specification
The service configuration contains both a
buildsection and animagetag, which creates ambiguity about which one will be used. Docker Compose will use thebuildconfiguration, but retaining theimagetag may lead to confusion.build: dockerfile: ./dev/docker/Dockerfile-cli context: ../../ - image: ghcr.io/xmtp/xmtpd-cli:sha-a96bf04Likely an incorrect or invalid review comment.
pkg/registry/contractRegistry.go (3)
238-238: LGTM - Updated field name to match contract structureThe field name has been updated from
SigningKeyPubtoSigningPublicKeyto align with changes in the contract structure.
248-250: LGTM - Updated field name for clarityRenaming from
InCanonicalNetworktoIsCanonicalimproves readability and follows common boolean field naming conventions.
252-258: LGTM - Updated Node struct fields to match the simplified data modelThe Node struct fields have been properly updated to match the simplified data model in
pkg/registry/node.go. TheMinMonthlyFeeMicroDollarsfield has been removed, and field names have been standardized.pkg/fees/contractRates_test.go (6)
34-35: LGTM - Updated to use interface type instead of concrete typeGood change to use the interface type
IRateRegistryRatesinstead of the concrete struct type for better abstraction and flexibility.
49-50: LGTM - Consistent use of interface typeThe test mock is correctly updated to return the interface type for better consistency with the refactored code.
67-68: LGTM - Consistent use of interface typeProperly updated to use the interface type in test mocks.
72-73: LGTM - Consistent use of interface typeProperly updated to use the interface type in test mocks.
89-94: LGTM - Consistent use of interface typeThe mock response correctly uses the interface type for the rates array.
125-126: LGTM - Consistent use of interface typeEmpty array is correctly typed as the interface type.
pkg/blockchain/migrator/migrator.go (3)
30-30: Type migration from int64 to uint32 for NodeId.The code now uses
node.NodeIddirectly instead of converting it toInt64(). This is part of the larger migration fromint64touint32for node IDs throughout the codebase.
35-35: Field name update to match contract structure.Changed from accessing
SigningKeyPubtoSigningPublicKeyto match the updated field name in the contract structures.
65-70: Removed minimum monthly fee parameter from AddNode method.The
AddNodemethod call no longer includes the minimum monthly fee parameter, which has been removed from the interface as part of the larger effort to remove fee management features.pkg/registry/contractRegistry_test.go (3)
55-60: Node ID and field name updates to match contract structure.Changed
NodeIdto plain integer type and updated field references to match the new contract structures:
SigningKeyPub→SigningPublicKeyInCanonicalNetwork→IsCanonicalThese changes are consistent with the Node struct updates in
pkg/registry/node.go.
63-68: Consistent field name updates for second test node.Similar field name updates as the first node, maintaining consistency across the test data.
168-173: Type update from big.Int to uint32 for NodeId.Changed NodeId type to
uint32with an explicit cast, which is consistent with the type changes throughout the codebase. Also updated the field names to match the new contract structures.pkg/config/cliOptions.go (4)
34-34: Updated NodeId type from int64 to uint32.Changed the type of NodeId from
int64touint32to be consistent with the blockchain registry interfaces.
58-62: Removed minimum monthly fee from RegisterNodeOptions.The
MinMonthlyFeeMicroDollarsfield has been removed from theRegisterNodeOptionsstruct, which aligns with the removal of fee management features throughout the codebase.
67-67: Updated NodeId type from int64 to uint32 in NetworkAdminOptions.Changed the type of NodeId from
int64touint32to be consistent with the blockchain registry interfaces.
73-73: Updated NodeId type from int64 to uint32 in SetHttpAddressOptions.Changed the type of NodeId from
int64touint32to be consistent with the blockchain registry interfaces.pkg/blockchain/migrator/migrator_test.go (2)
66-66: Removed minimum monthly fee parameter from AddNode call.The
AddNodemethod call no longer includes the minimum monthly fee parameter, aligning with the interface changes in the blockchain registry admin.
71-74: Simplified node creation without fee-related fields.The
SerializableNodecreation inregisterRandomNodehas been simplified by removing theMinMonthlyFeeMicroDollarsfield, consistent with the removal of fee management features.pkg/testutils/contracts.go (2)
3-14: Good job simplifying importsThe imports have been properly reduced to only those needed after removing contract deployment functionality.
20-87: LGTM: Event building functionality preservedThe code that builds ABI-encoded event data and logs for
MessageSentandIdentityUpdateCreatedevents has been correctly maintained, ensuring test utilities can still generate event data without contract deployment.dev/local.env (3)
19-24: Settlement chain contract addresses updatedThe settlement chain contract addresses have been updated, and new environment variables for payer registry, report manager, and distribution manager have been added. These changes align with the removal of fee management features.
26-28: Application chain contract addresses updatedThe addresses for the group message broadcast and identity update broadcast contracts have been updated.
52-52: New validation service address addedThe MLS validation GRPC address has been added to support the updated system architecture.
pkg/blockchain/registryCaller.go (2)
15-19: Type change from int64 to uint32 for nodeIdThe interface methods now use
uint32instead ofint64for node IDs, which is a positive change as it more precisely represents the valid range of node IDs.
55-62: Direct parameter passing improvedThe updated
GetNodeimplementation now passes theuint32nodeId directly to the contract call without conversion tobig.Int, which simplifies the code.pkg/mocks/fees/mock_RatesContract.go (4)
30-31: Interface type now used instead of concrete typeThe mock
GetRatesmethod now returns the interface typeIRateRegistryRatesinstead of the concrete type, which improves code flexibility and follows best practices for interface-based design.
40-46: Type changes consistently appliedThe changes from concrete to interface types have been consistently applied throughout the mock implementation, ensuring compatibility with the actual code.
90-104: Consistent return type updatesThe return types in mock methods have been consistently updated to use the interface type, which is good for maintaining compatibility across the codebase.
1-2:Details
✅ Verification successful
Auto-generated code
This is an auto-generated mock file, so the changes reflect updates to the interfaces it's mocking. These changes should be verified against the actual interfaces to ensure consistency.
🏁 Script executed:
#!/bin/bash # Check the interface definition to ensure it matches the mock rg --type go -A 5 "type RatesContract interface" pkg/Length of output: 375
The mock’s
GetRatessignature should exactly mirror the real interface. Let’s extract it from the generated mock for comparison:
🏁 Script executed:
#!/bin/bash rg "GetRates" -A 5 pkg/mocks/fees/mock_RatesContract.goLength of output: 1841
Mock signature matches interface definition – no action required
I’ve verified that the
GetRates(opts *bind.CallOpts, fromIndex *big.Int) (struct{ Rates []rateregistry.IRateRegistryRates; HasMore bool }, error)signature inpkg/mocks/fees/mock_RatesContract.goexactly mirrors the real interface inpkg/fees/contractRates.go. The auto-generated mock is consistent with the source interface.cmd/cli/main.go (1)
55-69:addRatesOptionsvariable is never populatedBecause the
"add-rates"command was removed, this variable is always zero-valued, yet it is embedded in the returnedCLIobject. If the struct is printed/logged it will expose empty data that cannot be set by users.Either re-add the command or remove the variable to prevent future confusion.
pkg/blockchain/registryAdmin.go (1)
117-120:Details
✅ Verification successful
Verify ABI – does the generated binding really take
uint32?
AddToNetwork,RemoveFromNetwork, andSetHttpAddressnow pass a plainuint32to the contract.abigenusually maps all integer Solidity types to Go numeric primitives exceptuint256, so this is probably correct, but please confirm the generated signatures to avoid a subtle compile/runtime mismatch.
🏁 Script executed:
#!/bin/bash # Show the AddToNetwork method signature in the generated binding fd --type=f "NodeRegistry.go" | xargs grep -n "func (.*)AddToNetwork"Length of output: 69
🏁 Script executed:
#!/bin/bash # Search for AddToNetwork method signatures in the generated Go bindings rg --color=never -n "AddToNetwork" .Length of output: 1446
AddToNetwork ABI Parameter Confirmed as uint32
I verified in pkg/abi/noderegistry/NodeRegistry.go that the generated binding declares:func (*NodeRegistryTransactor) AddToNetwork(opts *bind.TransactOpts, nodeId_ uint32) (*types.Transaction, error)Passing a uint32 is therefore correct—no changes required.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
pkg/blockchain/registryAdmin.go (1)
210-242:⚠️ Potential issueSetMaxCanonical implementation has issues.
The function has three issues:
- It contains a TODO comment indicating incomplete implementation
- It doesn't use the limit parameter passed to the function
- The contract call doesn't include the limit parameter
func (n *nodeRegistryAdmin) SetMaxCanonical( ctx context.Context, limit uint8, ) error { - // TODO set the actual limit return ExecuteTransaction( ctx, n.signer, n.logger, n.client, func(opts *bind.TransactOpts) (*types.Transaction, error) { - return n.contract.UpdateMaxCanonicalNodes( + return n.contract.UpdateMaxCanonicalNodes( opts, + limit, ) },It appears the contract method
UpdateMaxCanonicalNodesshould accept the limit parameter, but it's not being passed. This needs to be fixed to ensure the limit value is properly set in the contract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/cli/main.go(11 hunks)dev/docker/docker-compose-register.yml(4 hunks)dev/docker/docker-compose.yml(1 hunks)pkg/blockchain/registryAdmin.go(8 hunks)pkg/blockchain/registryCaller.go(2 hunks)pkg/config/cliOptions.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- dev/docker/docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- dev/docker/docker-compose-register.yml
- pkg/config/cliOptions.go
- pkg/blockchain/registryCaller.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
cmd/cli/main.go (4)
pkg/config/cliOptions.go (4)
NetworkAdminOptions(73-76)MigrateNodesOptions(37-40)GetMaxCanonicalOptions(42-43)SetMaxCanonicalOptions(45-48)pkg/blockchain/migrator/migrator.go (2)
ImportNodesFromFile(91-107)WriteToRegistry(52-77)pkg/blockchain/client.go (1)
NewClient(17-19)pkg/blockchain/registryCaller.go (1)
NewNodeRegistryCaller(30-48)
🔇 Additional comments (9)
cmd/cli/main.go (6)
41-45: CLI struct updated with new options for canonical node management.The CLI struct has been updated to include new options for getting and setting maximum canonical nodes, which aligns with the contract migration to version 0.4.0.
71-72: New variables for max canonical node options.These variables correctly match the new CLI command options, following the established pattern for command option variables.
91-93: New CLI command for setting maximum canonical node limit.The new command is correctly integrated into the CLI parser, following the established pattern for admin commands.
288-308: Well-structured implementation of setMaxCanonical function.The function follows the established pattern for admin commands, retrieving the admin instance and calling the appropriate method.
421-442: Well-implemented getMaxCanonicalNodes function.The function correctly obtains the registry caller and calls the appropriate method to retrieve the maximum canonical nodes limit.
591-593: Command switch cases properly updated.The switch statement has been correctly updated to handle the new commands for max canonical node management.
Also applies to: 609-611
pkg/blockchain/registryAdmin.go (3)
26-29: Interface updated with uint32 node IDs and new SetMaxCanonical method.The node ID type has been narrowed from int64 to uint32, which is more appropriate for the expected range of values. The new SetMaxCanonical method has been added to support the 0.4.0 contract feature.
99-103: Updated logging with appropriate type.Logging has been updated to use zap.Uint32 instead of zap.Int64 to match the type change in the interface.
135-136: Logging calls updated to use Uint32.All logging calls have been consistently updated to use zap.Uint32 instead of zap.Int64, matching the node ID type change in the interface.
Also applies to: 168-169, 203-205
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.dockerignore(1 hunks)dev/docker/docker-compose-register.yml(4 hunks)dev/docker/docker-compose.yml(1 hunks)dev/docker/up(1 hunks)dev/local.env(1 hunks)pkg/blockchain/migrator/migrator.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- dev/docker/docker-compose.yml
- dev/docker/docker-compose-register.yml
- pkg/blockchain/migrator/migrator.go
🧰 Additional context used
🪛 Gitleaks (8.26.0)
dev/local.env
3-3: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
24-24: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
29-29: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (5)
dev/docker/up (1)
28-28: Appropriate addition of the--buildflag.Adding the
--buildflag ensures that Docker will rebuild the images before starting containers, which is useful when working with the updated Docker Compose configuration that uses a reusable build anchor and adds new services. This ensures that any changes to the Dockerfiles or source code are incorporated into the containers..dockerignore (1)
1-21: Improved Docker build efficiency with whitelist approachThe change from a blacklist to a whitelist approach in
.dockerignoreis a best practice that:
- Reduces Docker build times and image sizes
- Minimizes attack surface by ensuring only required files are included
- Prevents accidental inclusion of sensitive files
The configuration appropriately includes Go module files, source code, and essential directories while excluding test files.
dev/local.env (3)
9-13: Configuration updated for new 0.4.0 contractsThe contract addresses have been updated to match the 0.4.0 version of contracts mentioned in the PR description. New registry contract addresses have been added for:
- Payer Registry
- Payer Report Manager
- Distribution Manager
These additions align with the PR objective of updating contract bindings.
34-36: Signer and payer keys are set to the same accountYou've explicitly set both the signer and payer private keys to the same value (ANVIL_ACC_1). While this works for local development, consider if this is the intended configuration or if these should be different accounts with different permissions in production.
🧰 Tools
🪛 Gitleaks (8.26.0)
34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
18-22: Legacy contract variables maintained for backward compatibilityThese legacy contract variables (
XMTPD_CONTRACTS_*) reference the same addresses as their newer counterparts. This helps maintain backward compatibility with existing code while transitioning to the new contract structure.
mkysel
force-pushed
the
mkysel/migrate-to-040-contracts
branch
from
c048a61 to
2b2e7f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
dev/local.env (1)
3-36: Secure local private keys in production environments.The environment file contains private keys suitable for local development, but they should not be used in production environments. Consider implementing a more secure method for managing secrets in production environments.
🧰 Tools
🪛 Gitleaks (8.26.0)
3-3: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
24-24: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
29-29: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
cmd/cli/main.go (1)
111-113:⚠️ Potential issueFix error message to match command name.
The error message refers to "get-rates command" instead of "get-max-canonical-nodes command".
if _, err := parser.AddCommand("get-max-canonical-nodes", "Get max canonical nodes for network", "", &getMaxCanonicalOptions); err != nil { - return nil, fmt.Errorf("could not add get-rates command: %s", err) + return nil, fmt.Errorf("could not add get-max-canonical-nodes command: %s", err) }pkg/blockchain/registryAdmin.go (1)
210-215: Unused parameter in SetMaxCanonical implementation.The
limitparameter is accepted but not used in the contract call. There's a TODO comment indicating this is pending implementation.Verify whether the
limitparameter should be passed to the contract method. The TODO comment suggests this is awaiting implementation, but it's important to ensure this is intentional and tracked for future work.
🧹 Nitpick comments (1)
pkg/fees/contractRates_test.go (1)
59-60: Test skipped to align with pagination removal.The
TestCanPaginatetest has been skipped, which is consistent with the removal of pagination logic from the contract rates fetching code. Consider adding a brief comment explaining why the test is skipped, or removing it entirely if it's no longer relevant.- t.Skip() + t.Skip("Pagination logic has been removed in the 0.4.0 contract version")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
dev/gen/abiis excluded by!**/gen/**
📒 Files selected for processing (24)
cmd/cli/main.go(11 hunks)dev/docker/docker-compose-register.yml(4 hunks)dev/docker/docker-compose.yml(1 hunks)dev/docker/up(1 hunks)dev/local.env(1 hunks)go.mod(1 hunks)pkg/blockchain/migrator/migrator.go(1 hunks)pkg/blockchain/migrator/migrator_test.go(1 hunks)pkg/blockchain/ratesAdmin.go(0 hunks)pkg/blockchain/ratesAdmin_test.go(0 hunks)pkg/blockchain/registryAdmin.go(8 hunks)pkg/blockchain/registryAdmin_test.go(2 hunks)pkg/blockchain/registryCaller.go(2 hunks)pkg/config/cliOptions.go(2 hunks)pkg/fees/contractRates.go(3 hunks)pkg/fees/contractRates_test.go(6 hunks)pkg/mocks/fees/mock_RatesContract.go(2 hunks)pkg/registry/contractRegistry.go(2 hunks)pkg/registry/contractRegistry_test.go(3 hunks)pkg/registry/node.go(2 hunks)pkg/testutils/anvil/anvil.go(1 hunks)pkg/testutils/config.go(1 hunks)pkg/testutils/contractRegistry.go(1 hunks)pkg/testutils/registry/registry.go(1 hunks)
💤 Files with no reviewable changes (2)
- pkg/blockchain/ratesAdmin.go
- pkg/blockchain/ratesAdmin_test.go
✅ Files skipped from review due to trivial changes (3)
- pkg/testutils/config.go
- pkg/testutils/anvil/anvil.go
- dev/docker/docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/testutils/contractRegistry.go
- dev/docker/docker-compose-register.yml
- dev/docker/up
- pkg/registry/node.go
- pkg/testutils/registry/registry.go
- pkg/registry/contractRegistry.go
- pkg/config/cliOptions.go
- pkg/blockchain/migrator/migrator_test.go
- pkg/registry/contractRegistry_test.go
- pkg/blockchain/migrator/migrator.go
- pkg/fees/contractRates.go
- pkg/blockchain/registryCaller.go
- pkg/mocks/fees/mock_RatesContract.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
cmd/cli/main.go (4)
pkg/config/cliOptions.go (4)
NetworkAdminOptions(72-75)MigrateNodesOptions(37-40)GetMaxCanonicalOptions(42-42)SetMaxCanonicalOptions(44-47)pkg/blockchain/migrator/migrator.go (2)
ImportNodesFromFile(93-109)WriteToRegistry(54-79)pkg/blockchain/client.go (1)
NewClient(17-19)pkg/blockchain/registryCaller.go (1)
NewNodeRegistryCaller(30-48)
pkg/fees/contractRates_test.go (3)
pkg/fees/contractRates.go (1)
ContractRatesFetcher(37-45)pkg/mocks/fees/mock_RatesContract.go (2)
MockRatesContract(16-18)NewMockRatesContract(90-100)pkg/abi/rateregistry/RateRegistry.go (1)
IRateRegistryRates(33-39)
🪛 Gitleaks (8.26.0)
dev/local.env
3-3: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
24-24: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
29-29: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 GitHub Check: Lint-Go
pkg/fees/contractRates_test.go
[failure] 83-83:
File is not properly formatted (golines)
🪛 GitHub Actions: Lint
pkg/fees/contractRates_test.go
[error] 83-83: File is not properly formatted (golines)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test (Node)
- GitHub Check: Upgrade Tests
🔇 Additional comments (20)
go.mod (1)
19-19: Dependency moved to direct requirements.The
github.com/pkg/errorspackage was promoted from indirect to direct dependencies, indicating it's now explicitly imported somewhere in the codebase. This aligns with error handling enhancements in other files that might be using this package's error wrapping capabilities.pkg/blockchain/registryAdmin_test.go (3)
41-59: Method signature update properly reflected in test helper.The
addRandomNodehelper function has been correctly updated to remove theminMonthlyFeeMicroDollarsparameter, aligning with the contract binding changes that removed fee-related parameters from the interface. This is consistent with the broader refactoring to move away from fee management functionality.
68-71: Registry.AddNode call updated to match new interface.The call to
registry.AddNodehas been updated to remove the fee parameter, correctly aligning with the new contract interface that no longer includes fee management functionality.
79-81: Invalid owner test correctly updated.The test for invalid owner address has been properly updated to use the new method signature without the fee parameter.
pkg/fees/contractRates_test.go (3)
17-30: Improved test cleanup handling.The function has been improved by using
t.Cleanup(cancel)instead of returning the cancel function, which ensures cleanup will happen automatically when the test completes. This is a more robust approach that prevents resource leaks if the caller forgets to invoke the cancel function.
32-40: Updated return type to interface.The function now returns the interface type
rateregistry.IRateRegistryRatesrather than the concrete struct, which is a good practice that improves flexibility and aligns with the contract binding changes.
133-134: Error assertions updated to use substring checks.The error assertions have been updated to use
ErrorContainsinstead of direct equality checks, which is more robust because it's less brittle to changes in error message formatting.Also applies to: 141-142
dev/local.env (1)
9-13: Added new contract addresses for 0.4.0 compatibility.The file has been updated with new contract addresses for the 0.4.0 version, including new addresses for the Payer Registry, Payer Report Manager, and Distribution Manager. This is consistent with the contract binding updates mentioned in the PR objectives.
cmd/cli/main.go (6)
248-254: Change in error handling severity.The error handling has been changed from non-fatal (previously logged as an error) to fatal, which will terminate the program if nodes can't be retrieved from the registry. This is consistent with other error handling in the file, but represents a behavior change that could affect operations.
Ensure this stricter error handling is appropriate and won't cause unexpected termination in normal operational scenarios.
284-285: Error handling changed to fatal.Similar to other places in this file, error handling has changed from non-fatal to fatal when adding a node to the network.
Confirm that this more strict error handling is intended behavior, as it will terminate the program on error rather than allowing it to continue with logged errors.
288-308: New function to manage max canonical nodes.The implementation follows the standard pattern of other admin functions in the file and correctly uses the new
SetMaxCanonicalmethod from the admin interface.
421-442: New getter implementation for max canonical nodes.The implementation follows the standard pattern used for other registry data getters and correctly handles errors.
591-593: Added new CLI command handling.The CLI command handler for the new
set-max-canonicalcommand has been properly integrated into the main command switch.
609-611: Added new CLI command handling.The CLI command handler for the new
get-max-canonical-nodescommand has been properly integrated into the main command switch.pkg/blockchain/registryAdmin.go (6)
26-29: Updated interface with uint32 node IDs and new functionality.The interface has been appropriately updated to:
- Change node ID type from
int64touint32, which is more memory efficient- Add a new
SetMaxCanonicalmethod to manage the maximum canonical nodes limitThis change is consistent with the broader refactoring to remove fee management and add canonical node management.
99-103: Updated logging to use correct type.Logging has been updated to use
zap.Uint32for node IDs to match the type change, and the output maintains all necessary information.
108-121: Changed parameter type and removed BigInt conversion.The
nodeIdparameter has been changed fromint64touint32and the BigInt conversion has been removed. This is consistent with the interface changes and likely reflects changes in the underlying contract.
134-136: Updated logging with proper type.The logging statement has been updated to use the appropriate
zap.Uint32type.
222-224: Contract call doesn't use limit parameter.The contract method
UpdateMaxCanonicalNodesdoesn't accept any parameters, which is inconsistent with the interface that accepts alimitparameter.Confirm if this is the correct behavior or if the contract method needs to be updated to accept the limit parameter. If the contract doesn't support setting a specific limit yet, consider documenting this clearly.
227-239: Event parsing and logging for max canonical nodes update.The event parsing and logging for the maximum canonical nodes update is implemented correctly, capturing the event data and logging it with the appropriate type.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/testutils/contracts.go (1)
18-20: Add “test-only” disclaimer to hard-coded private keyThe static key is still undocumented. A short comment makes its purpose explicit and prevents accidental reuse in non-test code.
-const ( - LOCAL_PRIVATE_KEY = "ac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80" -) +// WARNING: Test fixture only – NEVER use in production. +const LOCAL_PRIVATE_KEY = "ac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80"🧰 Tools
🪛 Gitleaks (8.26.0)
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🧹 Nitpick comments (2)
pkg/testutils/contracts.go (2)
54-58: Avoid shadowing the importedabipackageThe local variable
abihides the imported package name, hurting readability and tooling.- abi, err := gm.GroupMessageBroadcasterMetaData.GetAbi() + contractABI, err := gm.GroupMessageBroadcasterMetaData.GetAbi()Update the subsequent references (
contractABI) accordingly, and replicate the change inBuildIdentityUpdateLog.
60-70: Minor nit: usebig.NewInt(int64(sequenceID))for clarityThe helper
topic2 := common.BigToHash(new(big.Int).SetUint64(sequenceID))works, but
big.NewIntis shorter and clearer:-topic2 := common.BigToHash(new(big.Int).SetUint64(sequenceID)) +topic2 := common.BigToHash(big.NewInt(int64(sequenceID)))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/testutils/contracts.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint-Go
pkg/testutils/contracts.go
[failure] 4-4:
File is not properly formatted (gofumpt)
🪛 GitHub Actions: Lint
pkg/testutils/contracts.go
[error] 4-4: File is not properly formatted (gofumpt)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Test (Node)
- GitHub Check: Upgrade Tests
- GitHub Check: Code Review
mkysel
force-pushed
the
mkysel/migrate-to-040-contracts
branch
from
bfd0d52 to
0f9d5c9
Compare
Migrate RateRegistry and NodeRegistry contracts to 0.4.0 version by removing fee-based management and introducing canonical node limits
📍Where to StartStart with the contract interface changes in RateRegistry.go and registryAdmin.go which define the core architectural changes. Changes since #810 opened
Macroscope summarized 36fcdfc. |
mkysel
changed the title
fbac
left a comment
fbac
left a comment
There was a problem hiding this comment.
In a subsequent PR we should update the upgrade tests. Hopefully for the last time.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
pkg/testutils/contracts.go (2)
19-20: Document test-only private keyThis hardcoded private key is intended for testing purposes only and should be explicitly marked as such.
const ( + // WARNING: This key is for development/testing purposes ONLY and MUST NOT be used in production. LOCAL_PRIVATE_KEY = "ac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80" )🧰 Tools
🪛 Gitleaks (8.26.0)
20-20: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
26-40: 🛠️ Refactor suggestionAdd safeguard against future ABI changes in event encoding
The current implementation assumes there is exactly one non-indexed field in the events. If the contract ABI changes in the future to include additional non-indexed fields, this would silently cause incorrect encoding.
var nonIndexed abi.Arguments for _, input := range inputs { if !input.Indexed { nonIndexed = append(nonIndexed, input) } } + // Verify we have exactly one non-indexed parameter as expected + if len(nonIndexed) != 1 { + return nil, fmt.Errorf("unexpected non-indexed field count for MessageSent: got %d, want 1", len(nonIndexed)) + } return nonIndexed.Pack(message)cmd/cli/main.go (1)
111-113:⚠️ Potential issueError message doesn't match the command
The error message refers to "get-rates command" instead of "get-max-canonical-nodes command".
if _, err := parser.AddCommand("get-max-canonical-nodes", "Get max canonical nodes for network", "", &getMaxCanonicalOptions); err != nil { - return nil, fmt.Errorf("could not add get-rates command: %s", err) + return nil, fmt.Errorf("could not add get-max-canonical-nodes command: %s", err) }pkg/blockchain/registryAdmin.go (1)
210-215:⚠️ Potential issueUnused parameter in
SetMaxCanonicalmethodThe method accepts a
limitparameter (line 212) but doesn't use it when callingUpdateMaxCanonicalNodes. The TODO comment at line 214 suggests this is a known issue.Either the contract method should be updated to accept the limit parameter, or the method signature should be changed to not accept a limit. The current implementation could lead to confusion and bugs when the limit parameter is provided but has no effect.
func (n *nodeRegistryAdmin) SetMaxCanonical( ctx context.Context, - limit uint8, ) error { - // TODO set the actual limit + // Note: The contract method doesn't currently accept a limit parameter, + // so the max canonical nodes value is hardcoded in the contract. return ExecuteTransaction(Alternatively, if the contract is expected to be updated to support the limit parameter:
func (n *nodeRegistryAdmin) SetMaxCanonical( ctx context.Context, limit uint8, ) error { - // TODO set the actual limit + // TODO: The contract method doesn't currently accept a limit parameter. + // When the contract is updated, modify the call below to pass the limit parameter. return ExecuteTransaction(
🧹 Nitpick comments (4)
pkg/testutils/contracts.go (2)
107-121: Update misleading comment in event log constructionThe comment on line 117 refers to
groupIdbut in theIdentityUpdateLogfunction, this should beinboxId.return types.Log{ Topics: []common.Hash{ topic0, // event signature - topic1, // groupId + topic1, // inboxId topic2, // sequenceId }, Data: eventData, // ABI-encoded `message` (non-indexed) }
119-120: Update misleading comment for data fieldThe comment on the data field says
ABI-encoded 'message'but in theIdentityUpdateLogfunction, this should beupdate.}, - Data: eventData, // ABI-encoded `message` (non-indexed) + Data: eventData, // ABI-encoded `update` (non-indexed)pkg/blockchain/registryAdmin.go (1)
232-233: Update error message to match variable nameThe error message uses "NodeRegistryMaxCanonicalNodesUpdated" but should use "MaxCanonicalUpdated" to match the variable name used on line 230.
if !ok { n.logger.Error( - "unexpected event type, not of type NodeRegistryMaxCanonicalNodesUpdated", + "unexpected event type, not of type MaxCanonicalNodesUpdated", ) return }pkg/mocks/fees/mock_RatesContract.go (1)
88-116: Helpful addition ofGetRatesCountmock for paginationIntroducing
GetRatesCountis essential for the new paging logic; the generated helper methods are consistent withmockerypatterns.One nit you might consider (non-blocking): the panic on unset return values can sometimes be noisy when a test only cares about the call side-effects. If that becomes painful, you could wrap the generator in a custom template that defaults to zero-values instead of panicking, but the current behaviour is the safest default.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
dev/gen/abiis excluded by!**/gen/**
📒 Files selected for processing (24)
cmd/cli/main.go(11 hunks)dev/docker/docker-compose.yml(1 hunks)dev/local.env(2 hunks)go.mod(1 hunks)pkg/blockchain/migrator/migrator.go(1 hunks)pkg/blockchain/migrator/migrator_test.go(1 hunks)pkg/blockchain/ratesAdmin.go(0 hunks)pkg/blockchain/ratesAdmin_test.go(0 hunks)pkg/blockchain/registryAdmin.go(8 hunks)pkg/blockchain/registryAdmin_test.go(2 hunks)pkg/blockchain/registryCaller.go(2 hunks)pkg/config/cliOptions.go(2 hunks)pkg/fees/contractRates.go(5 hunks)pkg/fees/contractRates_test.go(4 hunks)pkg/mocks/fees/mock_RatesContract.go(2 hunks)pkg/registry/contractRegistry.go(2 hunks)pkg/registry/contractRegistry_test.go(3 hunks)pkg/registry/node.go(2 hunks)pkg/testutils/anvil/anvil.go(1 hunks)pkg/testutils/config.go(1 hunks)pkg/testutils/contractRegistry.go(1 hunks)pkg/testutils/contracts.go(4 hunks)pkg/testutils/registry/registry.go(1 hunks)pkg/upgrade/upgrade_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- pkg/blockchain/ratesAdmin.go
- pkg/blockchain/ratesAdmin_test.go
✅ Files skipped from review due to trivial changes (1)
- dev/docker/docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (16)
- go.mod
- pkg/testutils/anvil/anvil.go
- pkg/testutils/config.go
- pkg/registry/node.go
- pkg/blockchain/registryAdmin_test.go
- pkg/testutils/registry/registry.go
- pkg/testutils/contractRegistry.go
- pkg/registry/contractRegistry.go
- pkg/blockchain/migrator/migrator.go
- pkg/registry/contractRegistry_test.go
- pkg/fees/contractRates_test.go
- pkg/fees/contractRates.go
- pkg/config/cliOptions.go
- pkg/blockchain/registryCaller.go
- pkg/blockchain/migrator/migrator_test.go
- dev/local.env
🧰 Additional context used
🧬 Code Graph Analysis (2)
pkg/testutils/contracts.go (3)
pkg/abi/groupmessagebroadcaster/GroupMessageBroadcaster.go (1)
GroupMessageBroadcasterMetaData(33-36)pkg/utils/chainEvent.go (1)
GetEventTopic(21-28)pkg/abi/identityupdatebroadcaster/IdentityUpdateBroadcaster.go (1)
IdentityUpdateBroadcasterMetaData(33-36)
pkg/mocks/fees/mock_RatesContract.go (1)
pkg/abi/rateregistry/RateRegistry.go (1)
IRateRegistryRates(33-39)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test (Node)
🔇 Additional comments (2)
pkg/upgrade/upgrade_test.go (1)
12-13: Test upgrade path disabled - is this intentional?The
xmtpdVersionsslice has been emptied, effectively disabling theTestUpgradeToLatesttest since it will no longer iterate through any previous versions. This appears to intentionally remove testing for upgrade compatibility with older versions like 0.3.0.Is this intentional as part of the migration to 0.4.0? If so, consider adding a comment explaining why upgrade testing from previous versions is no longer needed:
var ( - xmtpdVersions = []string{} + // Previously contained "0.3.0" - intentionally disabled as 0.4.0 introduces breaking changes + // that make direct upgrades from previous versions unsupported + xmtpdVersions = []string{}pkg/mocks/fees/mock_RatesContract.go (1)
28-56: Mock signature correctly tracks new pagination contract – LGTMThe mock now mirrors the on-chain
GetRatessignature (adds thecountarg and returns[]rateregistry.IRateRegistryRates, error).
All call/return helpers were updated consistently, so existing unit tests that relied on the old struct return will fail fast and need updating—exactly what we want after an interface break.No functional or type-safety issues spotted in this block.
| func BuildIdentityUpdateEvent(update []byte) ([]byte, error) { | ||
| iuabi, err := iu.IdentityUpdateBroadcasterMetaData.GetAbi() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return abi.Events["IdentityUpdateCreated"].Inputs.Pack(inboxId, update, sequenceID) | ||
|
|
||
| inputs := iuabi.Events["IdentityUpdateCreated"].Inputs | ||
| var nonIndexed abi.Arguments | ||
| for _, input := range inputs { | ||
| if !input.Indexed { | ||
| nonIndexed = append(nonIndexed, input) | ||
| } | ||
| } | ||
|
|
||
| return nonIndexed.Pack(update) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safeguard against future ABI changes in event encoding (duplicate pattern)
Similar to the BuildMessageSentEvent function, this implementation assumes there is exactly one non-indexed field in the event. The same safeguard should be added here.
var nonIndexed abi.Arguments
for _, input := range inputs {
if !input.Indexed {
nonIndexed = append(nonIndexed, input)
}
}
+ // Verify we have exactly one non-indexed parameter as expected
+ if len(nonIndexed) != 1 {
+ return nil, fmt.Errorf("unexpected non-indexed field count for IdentityUpdateCreated: got %d, want 1", len(nonIndexed))
+ }
return nonIndexed.Pack(update)📝 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.
| func BuildIdentityUpdateEvent(update []byte) ([]byte, error) { | |
| iuabi, err := iu.IdentityUpdateBroadcasterMetaData.GetAbi() | |
| if err != nil { | |
| return nil, err | |
| } | |
| return abi.Events["IdentityUpdateCreated"].Inputs.Pack(inboxId, update, sequenceID) | |
| inputs := iuabi.Events["IdentityUpdateCreated"].Inputs | |
| var nonIndexed abi.Arguments | |
| for _, input := range inputs { | |
| if !input.Indexed { | |
| nonIndexed = append(nonIndexed, input) | |
| } | |
| } | |
| return nonIndexed.Pack(update) | |
| func BuildIdentityUpdateEvent(update []byte) ([]byte, error) { | |
| iuabi, err := iu.IdentityUpdateBroadcasterMetaData.GetAbi() | |
| if err != nil { | |
| return nil, err | |
| } | |
| inputs := iuabi.Events["IdentityUpdateCreated"].Inputs | |
| var nonIndexed abi.Arguments | |
| for _, input := range inputs { | |
| if !input.Indexed { | |
| nonIndexed = append(nonIndexed, input) | |
| } | |
| } | |
| // Verify we have exactly one non-indexed parameter as expected | |
| if len(nonIndexed) != 1 { | |
| return nil, fmt.Errorf( | |
| "unexpected non-indexed field count for IdentityUpdateCreated: got %d, want 1", | |
| len(nonIndexed), | |
| ) | |
| } | |
| return nonIndexed.Pack(update) | |
| } |
🤖 Prompt for AI Agents
In pkg/testutils/contracts.go around lines 75 to 89, the
BuildIdentityUpdateEvent function assumes exactly one non-indexed field in the
event inputs, which may break if the ABI changes. Add a safeguard by checking
that the length of nonIndexed is exactly one before packing, and return an error
if this condition is not met, similar to the pattern used in
BuildMessageSentEvent.
| func setMaxCanonical(logger *zap.Logger, options *CLI) { | ||
| ctx := context.Background() | ||
| registryAdmin, err := setupRegistryAdmin( | ||
| ctx, | ||
| logger, | ||
| options.NetworkAdminOptions.AdminOptions.AdminPrivateKey, | ||
| options.SetMaxCanonicalOptions.AdminOptions.AdminPrivateKey, | ||
| options.Contracts.SettlementChain.ChainID, | ||
| options, | ||
| ) | ||
| if err != nil { | ||
| logger.Fatal("could not setup registry admin", zap.Error(err)) | ||
| } | ||
|
|
||
| err = registryAdmin.RemoveFromNetwork( | ||
| ctx, | ||
| options.NetworkAdminOptions.NodeId, | ||
| ) | ||
| if err != nil { | ||
| logger.Fatal("could not remove node from network", zap.Error(err)) | ||
| } | ||
| } | ||
|
|
||
| func migrateNodes(logger *zap.Logger, options *CLI) { | ||
| ctx := context.Background() | ||
| nodes, err := migrator.ImportNodesFromFile(options.MigrateNodes.InFile) | ||
| if err != nil { | ||
| logger.Fatal("could not import nodes from file", zap.Error(err)) | ||
| } | ||
|
|
||
| newRegistryAdmin, err := setupRegistryAdmin( | ||
| err = registryAdmin.SetMaxCanonical( | ||
| ctx, | ||
| logger, | ||
| options.MigrateNodes.AdminOptions.AdminPrivateKey, | ||
| options.Contracts.SettlementChain.ChainID, | ||
| options, | ||
| options.SetMaxCanonicalOptions.Limit, | ||
| ) | ||
| if err != nil { | ||
| logger.Fatal("could not setup registry admin", zap.Error(err)) | ||
| } | ||
|
|
||
| err = migrator.WriteToRegistry(logger, nodes, newRegistryAdmin) | ||
| if err != nil { | ||
| logger.Fatal("could not write nodes to registry", zap.Error(err)) | ||
| logger.Fatal("could not set max canonical", zap.Error(err)) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Unused parameter in setMaxCanonical function
The function accepts a limit parameter but doesn't use it when calling UpdateMaxCanonicalNodes. The TODO comment suggests this is a known issue, but it should be better documented or fixed.
The contract method UpdateMaxCanonicalNodes doesn't appear to accept any parameters, but the CLI interface and function signature suggest a limit value should be passed. Is this a temporary situation until the contract is updated? Please verify if:
- The contract method should be updated to accept the limit parameter
- The CLI interface should be changed to not accept a limit parameter
- This is an intentional staging area for a future contract update
🏁 Script executed:
#!/bin/bash
# Check if the contract ABI actually supports a limit parameter
cat pkg/abi/noderegistry/NodeRegistry.go | grep -A 10 "func.*UpdateMaxCanonicalNodes"Length of output: 1470
🏁 Script executed:
#!/bin/bash
# Locate the setupRegistryAdmin function
rg -n "func setupRegistryAdmin" -n .Length of output: 85
🏁 Script executed:
#!/bin/bash
# Display the setupRegistryAdmin function definition and its return type
sed -n '600,700p' cmd/cli/main.goLength of output: 1244
🏁 Script executed:
#!/bin/bash
# Search for the SetMaxCanonical method in the blockchain package
rg -n "SetMaxCanonical" -n pkgLength of output: 268
🏁 Script executed:
#!/bin/bash
# Show the implementation of SetMaxCanonical in registryAdmin.go
sed -n '200,260p' pkg/blockchain/registryAdmin.goLength of output: 1046
Fix missing usage of limit in SetMaxCanonical
The CLI and admin implementation accept a limit but never use it when invoking the contract (per the // TODO set the actual limit comment). As a result, updateMaxCanonicalNodes() is always called with no parameters. To resolve this:
• Update the Solidity contract’s updateMaxCanonicalNodes signature to accept a uint8 maxCanonical and regenerate the Go bindings (ABI) so UpdateMaxCanonicalNodes(opts, limit) is available.
• In pkg/blockchain/registryAdmin.go, change:
return n.contract.UpdateMaxCanonicalNodes(
opts,
)to:
return n.contract.UpdateMaxCanonicalNodes(
opts,
limit,
)• In cmd/cli/main.go, ensure setMaxCanonical continues to pass options.SetMaxCanonicalOptions.Limit through.
• If the contract won’t support a parameter yet, remove the Limit CLI flag and the limit uint8 parameter from both setMaxCanonical and registryAdmin.SetMaxCanonical, or add a clear comment referencing a tracking issue for when the contract is updated.
Affected files:
- cmd/cli/main.go (
setMaxCanonical) - pkg/config/cliOptions.go (
SetMaxCanonicalOptions.Limit) - pkg/blockchain/registryAdmin.go (
SetMaxCanonical)
🤖 Prompt for AI Agents
In cmd/cli/main.go lines 288-308, the setMaxCanonical function accepts a limit
parameter but does not use it when calling registryAdmin.SetMaxCanonical, which
currently calls the contract method without parameters. To fix this, first
confirm if the Solidity contract's UpdateMaxCanonicalNodes method has been
updated to accept a limit parameter and regenerate the Go bindings. Then, update
pkg/blockchain/registryAdmin.go to pass the limit argument to
UpdateMaxCanonicalNodes. Finally, ensure setMaxCanonical in cmd/cli/main.go
passes options.SetMaxCanonicalOptions.Limit to registryAdmin.SetMaxCanonical. If
the contract does not yet support the parameter, remove the limit parameter and
CLI flag or add a clear comment referencing a tracking issue for future updates.
Migrate RateRegistry and NodeRegistry contracts to 0.4.0 version with parameter registry integration and updated data types
The pull request updates contract bindings and associated Go code to work with version 0.4.0 of the smart contracts. Key changes include:
RateRegistrycontract binding in pkg/abi/rateregistry/RateRegistry.go with new parameter registry integration and rate management functionsint64touint32across multiple packages including pkg/blockchain/registryAdmin.go and pkg/config/cliOptions.goPayerRegistryandPayerReportManagercontracts with associated Go bindings📍Where to Start
Start with the updated contract binding in pkg/abi/rateregistry/RateRegistry.go which contains the core interface changes for the 0.4.0 contract version.
Macroscope summarized 8809e5a.
Summary by CodeRabbit
New Features
Bug Fixes
Chores