Add parameter registry ABI and implement set maxcanonical #830
Conversation
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. |
mkysel
changed the title
WalkthroughA new Ethereum smart contract binding for Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as nodeRegistryAdmin
participant ParamReg as SettlementChainParameterRegistry
participant NodeReg as NodeRegistry
Admin->>ParamReg: Set0(key, value) (set max canonical nodes parameter)
Note right of ParamReg: Emits ParameterSet event
Admin->>NodeReg: UpdateMaxCanonicalNodes(limit)
alt NoChange error
Admin->>Admin: Log "no update needed"
else Success
Note right of NodeReg: Emits MaxCanonicalNodesUpdated event
end
sequenceDiagram
participant Test as TestSetMaxCanonical
participant Admin as nodeRegistryAdmin
Test->>Admin: SetMaxCanonical(ctx, 16)
Admin-->>Test: nil (success)
Test->>Admin: SetMaxCanonical(ctx, 16)
Admin-->>Test: nil (idempotent success)
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 (
|
| var value [32]byte | ||
| // store uint8 in the last byte for big-endian compatibility | ||
| value[31] = limit |
There was a problem hiding this comment.
FYI @deluca-mike. This is the endian-ness we need
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
pkg/blockchain/registryAdmin.go (5)
22-24: Consider shortening / hashing the parameter‐key to avoid oversized topic hashing
NODE_REGISTRY_MAX_CANONICAL_NODES_KEYis 35 bytes long ("xmtp.nodeRegistry.maxCanonicalNodes").
Althoughbytesis allowed as the indexed event parameter inSettlementChainParameterRegistry, the ABI encoder will hash the entire value to fit into a single topic (32 bytes).
Long keys therefore provide no extra distinguishability on-chain, but incur extra gas when used elsewhere (e.g. as storage keys or map look-ups).If backwards-compatibility isn’t required, think about:
-const NODE_REGISTRY_MAX_CANONICAL_NODES_KEY = "xmtp.nodeRegistry.maxCanonicalNodes" +// keccak256("xmtp.nodeRegistry.maxCanonicalNodes") truncated / encoded +var nodeRegistryMaxCanonicalKey = crypto.Keccak256([]byte("xmtp.nodeRegistry.maxCanonicalNodes"))…and re-using the
nodeRegistryMaxCanonicalKeybyte slice everywhere.
55-70: Avoid repeated contract instantiation logicBoth contract instances are created with almost identical boiler-plate.
Extracting a small helper function reduces duplication, centralises error handling, and simplifies future upgrades:+func mustBind[T any](bindFn func(common.Address, *ethclient.Client) (T, error), addr string, client *ethclient.Client) (T, error) { + return bindFn(common.HexToAddress(addr), client) +} ... -nodeContract, err := noderegistry.NewNodeRegistry( - common.HexToAddress(contractsOptions.SettlementChain.NodeRegistryAddress), - client, -) +nodeContract, err := mustBind(noderegistry.NewNodeRegistry, + contractsOptions.SettlementChain.NodeRegistryAddress, client) ... -paramContract, err := paramReg.NewSettlementChainParameterRegistry( - common.HexToAddress(contractsOptions.SettlementChain.ParameterRegistryAddress), - client, -) +paramContract, err := mustBind(paramReg.NewSettlementChainParameterRegistry, + contractsOptions.SettlementChain.ParameterRegistryAddress, client)This keeps
NewNodeRegistryAdminreadable and uniform.
231-244: Pre-allocate immutable data to save per-call allocations
key := []byte(NODE_REGISTRY_MAX_CANONICAL_NODES_KEY)allocates on every invocation.
Store the byte slice as avaralongside the constant:var nodeRegistryMaxCanonicalKey = []byte(NODE_REGISTRY_MAX_CANONICAL_NODES_KEY) ... key := nodeRegistryMaxCanonicalKeyThis shaves a small amount of work from what may become a hot path in administration tooling.
292-300: “NoChange” error detection is brittleString-contains checks on error text are fragile and may break with upstream changes.
Prefer a typed error or inspect the revert data directly if the bindings expose one.If that is not available, at least compare against a constant:
const errNoChange = "NoChange" ... if errors.Is(err, errNoChange) || strings.Contains(err.Error(), errNoChange) { ... }
266-275: Potential redundant on-chain transaction
UpdateMaxCanonicalNodes()is called unconditionally after setting the parameter, even if the parameter value hasn’t changed.
You already swallow the “NoChange” revert, but that still pays gas for a failed transaction.Consider optimising:
- Call
parameterContract.Get0first to see whetherlimitdiffers.- Only fire
Set0/UpdateMaxCanonicalNodeswhen necessary.This preserves idempotency while avoiding wasted gas.
pkg/abi/settlementchainparameterregistry/SettlementChainParameterRegistry.go (1)
1-36: Generated file – add linter directivesBecause this file is generated, linters such as
staticcheckorgolangci-lintmay flag stylistic issues outside your control.
Add the conventional directive at the top to silence them:// Code generated - DO NOT EDIT. //go:build !lint // +build !lint(or whichever tag your tooling respects).
This keeps CI noise low and focuses reviewers on hand-written code.
📜 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 (6)
dev/local.env(1 hunks)pkg/abi/settlementchainparameterregistry/SettlementChainParameterRegistry.go(1 hunks)pkg/blockchain/registryAdmin.go(10 hunks)pkg/blockchain/registryAdmin_test.go(1 hunks)pkg/config/options.go(1 hunks)pkg/testutils/config.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Upgrade Tests
- GitHub Check: Test (Node)
🔇 Additional comments (6)
dev/local.env (1)
24-24: LGTM: Added new Parameter Registry address environment variable.The addition of the
XMTPD_SETTLEMENT_CHAIN_PARAMETER_REGISTRY_ADDRESSenvironment variable is consistent with the existing pattern for settlement chain contracts.pkg/config/options.go (1)
49-49: LGTM: Added Parameter Registry address configuration.The new
ParameterRegistryAddressfield in theSettlementChainOptionsstruct properly follows the established pattern for contract address configurations and correctly maps to the environment variable added inlocal.env.pkg/blockchain/registryAdmin_test.go (1)
84-93: LGTM: Good test coverage for the SetMaxCanonical functionality.The test verifies that the
SetMaxCanonicalmethod works correctly and handles repeated calls with the same value without errors. It follows the existing testing patterns in the file.pkg/testutils/config.go (2)
21-21: LGTM: Added Parameter Registry address constant.The new constant for the Parameter Registry address is correctly defined and follows the established naming pattern.
40-40: LGTM: Updated test configuration with Parameter Registry address.The
NewContractsOptionsfunction is properly updated to include the Parameter Registry address in the returned configuration.pkg/blockchain/registryAdmin.go (1)
71-77: Field ordering hintPlacing fields that are frequently accessed together next to each other (e.g.
nodeContract,parameterContract) can improve cache locality and readability. Consider grouping the two contract handles.
Implement parameter registry ABI and SetMaxCanonical method to set maximum canonical nodes in the node registry using SettlementChainParameterRegistry contract
Adds support for the
SettlementChainParameterRegistrycontract by:SetMaxCanonicalmethod in registryAdmin.go to set maximum canonical nodes parameter using the new parameter registry contractSettlementChainParameterRegistrycontract in SettlementChainParameterRegistry.goSetMaxCanonicalfunctionality in registryAdmin_test.go📍Where to Start
Start with the
SetMaxCanonicalmethod implementation in registryAdmin.go which shows how the parameter registry contract is used to set the maximum canonical nodes parameter.Macroscope summarized ae54a96.
Summary by CodeRabbit