Add multi-chain support #755
Conversation
|
Warning Rate limit exceeded@fbac has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (28)
## Walkthrough
The changes restructure contract and blockchain configuration throughout the codebase by introducing nested `AppChain` and `SettlementChain` structs within `ContractsOptions`. All references to contract addresses, chain IDs, RPC URLs, and refresh intervals are updated to use these new nested fields. Associated test and validation logic are also adjusted accordingly.
## Changes
| Files/Groups | Change Summary |
|---------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `pkg/config/options.go` | Refactored `ContractsOptions` from a flat structure to include nested `AppChainOptions` and `SettlementChainOptions` for clearer separation of configuration parameters for two chains. |
| `pkg/config/validation.go` | Updated validation logic to separately validate nested `AppChain` and `SettlementChain` fields, replacing previous flat field checks. |
| `cmd/cli/main.go`, `cmd/replication/main.go`, `pkg/server/server.go` | Updated blockchain client and signer initialization to use nested `AppChain` or `SettlementChain` fields for RPC URLs and chain IDs. Variable names updated for clarity. |
| `pkg/blockchain/blockchainPublisher.go`, `pkg/blockchain/blockchainPublisher_test.go` | Changed contract address and chain ID references from flat to nested `AppChain`/`SettlementChain` fields in publisher logic and tests. |
| `pkg/blockchain/migrator/migrator_test.go`, `pkg/blockchain/registryAdmin_test.go` | Updated test setup to use nested `SettlementChain` fields for node registry contract addresses, chain IDs, and RPC URLs. |
| `pkg/blockchain/ratesAdmin.go`, `pkg/blockchain/ratesAdmin_test.go`, `pkg/fees/contractRates.go` | Changed rate registry contract address and refresh interval references to use `SettlementChain` nested fields in both implementation and tests. |
| `pkg/blockchain/registryAdmin.go`, `pkg/blockchain/registryCaller.go` | Updated node registry contract address references to use nested `SettlementChain` fields in admin/caller constructors. |
| `pkg/indexer/indexer.go`, `pkg/indexer/e2e_test.go`, `pkg/indexer/storer/groupMessage_test.go`, `pkg/indexer/storer/identityUpdate_test.go` | Updated all contract address, chain ID, and RPC URL references to use `AppChain` nested fields in indexer logic and tests. |
| `pkg/registry/contractRegistry.go`, `pkg/registry/contractRegistry_test.go` | Changed node registry address and refresh interval configuration to use nested `SettlementChain` fields in registry logic and tests. |
| `pkg/server/server_test.go` | Modified test server configuration to use nested `AppChain` fields for RPC URL and disconnect time. |
| `pkg/testutils/config.go` | Changed utility function to return `ContractsOptions` with nested `AppChain` and `SettlementChain` fields instead of flat fields. |
| `pkg/utils/namespace.go` | Updated namespace derivation to use `SettlementChain.NodeRegistryAddress` instead of flat contract address. |
| `dev/local.env`, `pkg/upgrade/scripts/load_env.sh` | Updated environment variables and script to replace single RPC URL variable with separate `APP_CHAIN` and `SETTLEMENT_CHAIN` RPC URL variables; renamed contract address variables to reflect new nested configuration structure. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Config as ContractsOptions
participant AppChain as AppChainOptions
participant SettlementChain as SettlementChainOptions
participant Client as Blockchain Client
participant Contract as Smart Contract
Config->>AppChain: Access AppChain fields (e.g. RpcURL, ChainID)
Config->>SettlementChain: Access SettlementChain fields (e.g. RpcURL, NodeRegistryAddress)
AppChain->>Client: Initialize client with AppChain.RpcURL
SettlementChain->>Client: Initialize client with SettlementChain.RpcURL
Client->>Contract: Instantiate contract with address from AppChain/SettlementChainPossibly related PRs
Suggested reviewers
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
cmd/cli/main.go(14 hunks)cmd/replication/main.go(1 hunks)pkg/blockchain/blockchainPublisher.go(2 hunks)pkg/blockchain/blockchainPublisher_test.go(1 hunks)pkg/blockchain/migrator/migrator_test.go(1 hunks)pkg/blockchain/ratesAdmin.go(1 hunks)pkg/blockchain/ratesAdmin_test.go(1 hunks)pkg/blockchain/registryAdmin.go(1 hunks)pkg/blockchain/registryAdmin_test.go(1 hunks)pkg/blockchain/registryCaller.go(1 hunks)pkg/config/options.go(1 hunks)pkg/config/validation.go(1 hunks)pkg/fees/contractRates.go(2 hunks)pkg/indexer/e2e_test.go(2 hunks)pkg/indexer/indexer.go(8 hunks)pkg/indexer/storer/groupMessage_test.go(1 hunks)pkg/indexer/storer/identityUpdate_test.go(1 hunks)pkg/registry/contractRegistry.go(2 hunks)pkg/registry/contractRegistry_test.go(3 hunks)pkg/server/server.go(2 hunks)pkg/server/server_test.go(1 hunks)pkg/testutils/config.go(1 hunks)pkg/utils/namespace.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (12)
cmd/replication/main.go (2)
pkg/blockchain/client.go (1)
NewClient(17-19)pkg/registry/contractRegistry.go (1)
NewSmartContractRegistry(53-79)
pkg/blockchain/blockchainPublisher_test.go (4)
pkg/testutils/contracts.go (3)
DeployNodesContract(178-180)DeployGroupMessagesContract(182-184)DeployIdentityUpdatesContract(186-188)pkg/blockchain/signer.go (1)
NewPrivateKeySigner(26-52)pkg/testutils/config.go (1)
GetPayerOptions(32-36)pkg/blockchain/client.go (1)
NewClient(17-19)
cmd/cli/main.go (2)
pkg/blockchain/client.go (1)
NewClient(17-19)pkg/blockchain/signer.go (1)
NewPrivateKeySigner(26-52)
pkg/blockchain/ratesAdmin_test.go (3)
pkg/testutils/contracts.go (2)
DeployRatesRegistryContract(190-192)LOCAL_PRIVATE_KEY(26-26)pkg/blockchain/signer.go (1)
NewPrivateKeySigner(26-52)pkg/blockchain/client.go (1)
NewClient(17-19)
pkg/server/server_test.go (1)
pkg/config/options.go (1)
AppChainOptions(17-25)
pkg/testutils/config.go (1)
pkg/config/options.go (2)
AppChainOptions(17-25)SettlementChainOptions(27-32)
pkg/indexer/e2e_test.go (3)
pkg/testutils/contracts.go (2)
DeployGroupMessagesContract(182-184)DeployIdentityUpdatesContract(186-188)pkg/blockchain/signer.go (1)
NewPrivateKeySigner(26-52)pkg/blockchain/client.go (1)
NewClient(17-19)
pkg/blockchain/registryAdmin_test.go (4)
pkg/testutils/contracts.go (1)
DeployNodesContract(178-180)pkg/blockchain/signer.go (1)
NewPrivateKeySigner(26-52)pkg/testutils/config.go (1)
GetPayerOptions(32-36)pkg/blockchain/client.go (1)
NewClient(17-19)
pkg/blockchain/migrator/migrator_test.go (4)
pkg/testutils/contracts.go (1)
DeployNodesContract(178-180)pkg/blockchain/signer.go (1)
NewPrivateKeySigner(26-52)pkg/testutils/config.go (1)
GetPayerOptions(32-36)pkg/blockchain/client.go (1)
NewClient(17-19)
pkg/server/server.go (1)
pkg/blockchain/client.go (1)
NewClient(17-19)
pkg/blockchain/blockchainPublisher.go (1)
pkg/abi/identityupdatebroadcaster/IdentityUpdateBroadcaster.go (1)
NewIdentityUpdateBroadcaster(123-129)
pkg/registry/contractRegistry_test.go (1)
pkg/config/options.go (2)
ContractsOptions(12-15)SettlementChainOptions(27-32)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Code Review
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Upgrade Tests
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Test (Node)
🔇 Additional comments (60)
pkg/blockchain/registryCaller.go (1)
33-33: LGTM: Address reference updated for multi-chain supportThe contract address reference has been correctly updated to use the nested
SettlementChain.NodeRegistryAddressfield instead of a top-level field, which aligns with the multi-chain support objective of this PR.cmd/replication/main.go (2)
102-105: LGTM: Client initialization updated for settlement chainThe blockchain client is now properly initialized with the settlement chain's RPC URL and renamed to
settlementChainClient, making it clear which chain this client is connected to.
112-112: LGTM: Chain-specific client referenceThe renamed client is correctly passed to the smart contract registry, maintaining consistency with the multi-chain architecture.
pkg/utils/namespace.go (1)
13-13: LGTM: Updated namespace generation for multi-chain supportThe namespace generation correctly uses the settlement chain's node registry address, maintaining namespace consistency while supporting the new multi-chain structure.
pkg/blockchain/registryAdmin.go (1)
52-52: LGTM: Contract address reference updated for multi-chain supportThe node registry contract address is now correctly sourced from the nested
SettlementChainstructure, consistent with the overall refactoring pattern in this PR.pkg/blockchain/ratesAdmin.go (1)
34-34: LGTM: Correctly updated to use nested AppChain configurationThe change properly updates the contract address reference to use the new nested
AppChainstruct, which is consistent with the multi-chain support refactoring.pkg/blockchain/blockchainPublisher.go (3)
52-54: LGTM: Properly updated contract address referenceThe GroupMessageBroadcaster address is now correctly accessed from the nested AppChain configuration.
59-61: LGTM: Properly updated contract address referenceThe IdentityUpdateBroadcaster address is now correctly accessed from the nested AppChain configuration.
83-83: LGTM: Updated logger context with new configuration pathLogger field updated to use the new nested path to the GroupMessageBroadcasterAddress.
pkg/registry/contractRegistry_test.go (3)
40-44: LGTM: Test configuration updated to use nested SettlementChain structureThe test is properly updated to use the new configuration structure, with the NodeRegistryRefreshInterval now under the nested SettlementChain options.
93-97: LGTM: Test configuration updated to use nested SettlementChain structureThis test case correctly uses the nested configuration structure with the refresh interval in the appropriate location.
155-159: LGTM: Test configuration updated to use nested SettlementChain structureThe TestStopOnContextCancel test has been properly updated to use the new nested configuration pattern.
pkg/indexer/storer/groupMessage_test.go (3)
27-30: LGTM: Test setup updated to use nested AppChain configurationThe contract address is now correctly assigned to the nested AppChain structure.
32-32: LGTM: Updated client initialization to use AppChain RPC URLThe blockchain client is now properly initialized using the RPC URL from the nested AppChain configuration.
35-35: LGTM: Updated contract address referenceContract initialization correctly uses the address from the nested AppChain configuration.
pkg/blockchain/migrator/migrator_test.go (3)
24-24: Node registry contract is now properly associated with the settlement chain.The code correctly updates the contract address storage to use the new nested
SettlementChainstructure. This aligns with the multi-chain support being added in this PR, where node registry functionality is consistently placed under the settlement chain.
28-28: Signer's chain ID now references settlement chain.The signer configuration correctly uses the settlement chain's ID for transaction signing. This ensures transactions to the node registry contract are properly signed for the correct blockchain.
32-32: Blockchain client now connects to settlement chain.The client instantiation properly uses the settlement chain's RPC URL, ensuring the test connects to the correct blockchain network for node registry operations.
pkg/blockchain/ratesAdmin_test.go (3)
24-27: Rate registry contract is now properly associated with the app chain.The code correctly updates the rate registry contract address to be under the
AppChainstruct. This is consistent with the architectural changes in this PR that separate app chain and settlement chain functionality.
31-31: Signer's chain ID now references app chain.The private key signer is correctly configured to use the app chain's ID, ensuring that transactions related to rates are signed for the appropriate blockchain.
35-35: Blockchain client now connects to app chain.The client initialization properly uses the app chain's RPC URL, ensuring the test connects to the correct blockchain network for rate registry operations.
pkg/server/server_test.go (1)
45-48: Test server configuration updated to use nested AppChain options.The server configuration for testing has been properly updated to use the new nested structure, with RPC URL and max disconnect time now correctly placed under the AppChain options. This aligns with the multi-chain support being implemented across the codebase.
pkg/blockchain/registryAdmin_test.go (3)
23-23: Node registry contract is now properly associated with the settlement chain.The code correctly updates the node registry contract address to use the new nested
SettlementChainstructure. This is consistent with the changes in other files that place node registry functionality under the settlement chain.
27-27: Signer's chain ID now references settlement chain.The private key signer is correctly configured to use the settlement chain's ID, ensuring that transactions to the node registry contract are signed for the appropriate blockchain.
31-31: Blockchain client now connects to settlement chain.The client instantiation properly uses the settlement chain's RPC URL, ensuring the test connects to the correct blockchain network for node registry operations.
pkg/server/server.go (3)
241-241: Chain ID source updated to use AppChain struct.The code now correctly uses the new nested AppChain configuration, making the chain distinction explicit.
247-247: Renamed variable and updated RPC URL source to use AppChain.Good change that improves clarity by:
- Renaming
ethclienttoappChainClientto better indicate its purpose- Using the new nested AppChain configuration structure
This makes it clear which chain this client is connecting to.
257-257: Updated blockchain publisher to use appChainClient.This correctly ensures the blockchain publisher uses the renamed client that's connected to the application chain.
pkg/indexer/e2e_test.go (3)
33-40: Updated contract address assignment to use AppChain nested structure.The test now correctly uses the restructured configuration, assigning the deployed contract addresses to the appropriate nested fields in the AppChain structure.
65-65: Updated chain ID reference to use AppChain nested field.Properly uses the new configuration structure for the blockchain signer setup.
69-69: Updated RPC URL reference to use AppChain nested field.Correctly uses the new configuration structure for client instantiation.
cmd/cli/main.go (5)
220-221: Updated to use SettlementChain's ChainID for registry admin setup.The code now correctly references the chain ID from the settlement chain configuration.
251-254: Updated RPC URL to use SettlementChain's configuration.The code now correctly creates a chain client using the settlement chain's RPC URL.
288-288: Updated chain ID references to use SettlementChain consistently.The code consistently references the settlement chain's chain ID across multiple administrative functions.
Also applies to: 315-315, 342-342, 361-361, 383-383, 409-409, 484-484, 507-507
402-402: Updated RPC URL references to use SettlementChain consistently.The code consistently references the settlement chain's RPC URL across blockchain client initialization points.
Also applies to: 445-445, 536-536, 571-571
725-728: Updated setupRegistryAdmin to use SettlementChain's RPC URL.The helper function correctly uses the settlement chain configuration for blockchain client setup.
pkg/indexer/storer/identityUpdate_test.go (3)
31-34: Updated contract address assignment to use AppChain configuration.The test now correctly deploys and assigns the contract address to the nested AppChain field.
36-36: Updated client initialization to use AppChain's RPC URL.Correctly references the AppChain's RPC URL for blockchain client creation.
39-39: Updated contract address reference to use AppChain configuration.The contract initialization now correctly uses the nested AppChain field for the contract address.
pkg/registry/contractRegistry.go (2)
60-60: Configuration path updated to use nested structure.The node registry contract address is now accessed through the
SettlementChainstruct, aligning with the multi-chain support architecture.
72-72: Refresh interval now accessed through nested SettlementChain struct.The refresh interval for the node registry is consistently updated to use the nested structure, maintaining alignment with the contract address change above.
pkg/indexer/indexer.go (9)
73-73: RPC URL now accessed through AppChain struct.The blockchain client initialization now correctly uses the AppChain's RPC URL, supporting the multi-chain architecture.
97-97: Contract address logging uses AppChain structure.The logger now consistently references the contract address through the AppChain struct, enhancing clarity about which chain this contract belongs to.
Also applies to: 108-108
123-123: Identity update broadcaster contract address updated to use AppChain.Similar to the group message broadcaster, the identity update broadcaster contract address is now properly referenced through the AppChain struct.
Also applies to: 138-138
170-172: Block tracker initialization updated for multi-chain support.The message tracker initialization now correctly uses the contract address from the AppChain struct, maintaining consistency with other changes.
180-183: ListenForContractEvent parameters updated for multi-chain architecture.The contract address and max disconnect time are now accessed through the AppChain struct, ensuring consistency across the codebase.
190-194: Identity updates tracker initialization updated.Similar to the messages tracker, the identity updates tracker now uses the appropriate contract address from the AppChain struct.
202-204: Identity update contract event parameters updated.The contract address and max disconnect time for identity updates are now consistently accessed through the AppChain struct.
410-411: Contract instantiation updated for multi-chain support.The message contract instantiation function now correctly references the contract address through the AppChain struct.
420-421: Identity update contract instantiation uses AppChain struct.Similarly to the message contract, the identity update contract instantiation now correctly uses the AppChain nested field.
pkg/fees/contractRates.go (2)
60-61: Rate registry contract address now accessed through AppChain struct.The rate registry contract address is now accessed through the AppChain nested structure, properly categorizing this contract as part of the application chain rather than the settlement chain.
71-71: Rates refresh interval accessed through AppChain struct.The refresh interval is consistently accessed through the same nested structure as the contract address, maintaining a clear architecture.
pkg/blockchain/blockchainPublisher_test.go (3)
21-29: Test contract deployments updated for multi-chain support.The test setup now correctly assigns contract addresses to the appropriate chain structures:
- Node registry to the SettlementChain
- Message broadcasters to the AppChain
This change ensures tests properly reflect the production code's multi-chain architecture.
33-33: Signer creation uses chain ID from AppChain struct.The signer is now correctly initialized with the chain ID from the AppChain, ensuring transactions are signed with the appropriate chain parameters.
37-37: Blockchain client uses RPC URL from SettlementChain.The client is now initialized with the RPC URL from the SettlementChain struct, which is correctly aligned with the test's use of the node registry on the settlement chain.
pkg/testutils/config.go (1)
18-28: Configuration refactored to support multi-chain - LGTM!The
NewContractsOptionsfunction has been correctly updated to use the new nested structure withAppChainandSettlementChainfields. Both chains share the same RPC URL and chain ID for testing purposes, which makes sense since most tests likely run against a single local blockchain instance.The test configuration provides reasonable defaults for both chains while maintaining the expected behavior of existing tests.
pkg/config/validation.go (2)
13-40: Validation logic split for AppChain fields - LGTM!The validation logic has been properly updated to check all required fields and constraints for the Application Chain configuration. Error messages correctly reference the new nested structure with appropriate field paths.
42-57: Validation logic for SettlementChain fields - LGTM!The validation logic properly checks all required fields for the Settlement Chain configuration, including RPC URL, chain ID, node registry address, and refresh interval. Error messages are clear and correctly reference the nested structure.
pkg/config/options.go (2)
13-15: ContractsOptions split into two chain-specific structs - LGTM!The
ContractsOptionsstruct has been restructured to contain two separate chain configurations, which better organizes the contract-related settings and provides clearer separation of concerns between application and settlement chains.
17-25: New AppChainOptions struct looks good!The
AppChainOptionsstruct properly encapsulates all application chain related configuration including RPC URL, chain ID, contract addresses, and intervals.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
fbac
force-pushed
the
05-06-support_multi_chain
branch
from
0d4b1ca to
8f4b43b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
doc/onboarding.md (1)
45-46: Disable MD034 lint for RPC URLs in code block
The bare RPC URL in the shell snippet triggers MD034. To suppress this for the entire block, add:<!-- markdownlint-disable MD034 -->immediately before the code fence.
doc/deploy.md (3)
15-19: Convert bare URLs in the DNS table to markdown links
Raw endpoints (e.g.,https://grpc.testnet.xmtp.network) should be wrapped in link syntax for clarity and to satisfy markdownlint (MD034).Example diff:
-| https://grpc.testnet.xmtp.network | US-EAST-2 | 0x03e... | +| [grpc.testnet.xmtp.network](https://grpc.testnet.xmtp.network) | US-EAST-2 | 0x03e... |🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
17-17: Bare URL used
null(MD034, no-bare-urls)
18-18: Bare URL used
null(MD034, no-bare-urls)
72-75: Mirror registry variable naming in verification section
Similar to registration, the verification example still referencesXMTPD_CONTRACTS_NODE_REGISTRY_ADDRESS. Update to:export XMTPD_SETTLEMENT_CHAIN_NODE_REGISTRY_ADDRESS="0xDEADBEEF"
85-106: Replace hard tabs in JSON sample
The JSON output block contains hard tabs, violating MD010. Convert tabs to spaces (e.g., 2 spaces per indent) for proper formatting.Example diff for the first few lines:
- "level": "INFO", - "time": "2025-05-06T16:39:35.737+0200", - "message": "got nodes", + "level": "INFO", + "time": "2025-05-06T16:39:35.737+0200", + "message": "got nodes",🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
85-85: Hard tabs
Column: 1(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 1(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 1(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 1(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 1(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 1(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 1(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 1(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 1(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 1(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 1(MD010, no-hard-tabs)
99-99: Hard tabs
Column: 1(MD010, no-hard-tabs)
100-100: Hard tabs
Column: 1(MD010, no-hard-tabs)
101-101: Hard tabs
Column: 1(MD010, no-hard-tabs)
102-102: Hard tabs
Column: 1(MD010, no-hard-tabs)
103-103: Hard tabs
Column: 1(MD010, no-hard-tabs)
104-104: Hard tabs
Column: 1(MD010, no-hard-tabs)
105-105: Hard tabs
Column: 1(MD010, no-hard-tabs)
106-106: Hard tabs
Column: 1(MD010, no-hard-tabs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
doc/deploy.md(1 hunks)doc/onboarding.md(2 hunks)pkg/blockchain/ratesAdmin.go(1 hunks)pkg/blockchain/ratesAdmin_test.go(1 hunks)pkg/config/options.go(1 hunks)pkg/config/validation.go(1 hunks)pkg/fees/contractRates.go(2 hunks)pkg/testutils/config.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/blockchain/ratesAdmin_test.go
- pkg/blockchain/ratesAdmin.go
- pkg/testutils/config.go
- pkg/fees/contractRates.go
- pkg/config/validation.go
- pkg/config/options.go
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
doc/deploy.md
17-17: Bare URL used
null
(MD034, no-bare-urls)
18-18: Bare URL used
null
(MD034, no-bare-urls)
85-85: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1
(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 1
(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 1
(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 1
(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 1
(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 1
(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 1
(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 1
(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 1
(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 1
(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 1
(MD010, no-hard-tabs)
99-99: Hard tabs
Column: 1
(MD010, no-hard-tabs)
100-100: Hard tabs
Column: 1
(MD010, no-hard-tabs)
101-101: Hard tabs
Column: 1
(MD010, no-hard-tabs)
102-102: Hard tabs
Column: 1
(MD010, no-hard-tabs)
103-103: Hard tabs
Column: 1
(MD010, no-hard-tabs)
104-104: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Code Review
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
doc/deploy.md (1)
78-79:⚠️ Potential issueInconsistent private key environment variable usage
Theget-all-nodescommand still references$PRIVATE_KEY, whereas the registration step uses${ADMIN_PRIVATE_KEY}(orXMTPD_ADMIN_PRIVATE_KEYif renamed). Update it to:dev/cmd/get-all-nodes \ --admin.private-key=${ADMIN_PRIVATE_KEY}
🧹 Nitpick comments (4)
doc/deploy.md (4)
17-18: Convert bare URLs to markdown links
Markdownlint flags bare URLs in the table header (lines 17–18). Wrapping them in link syntax improves readability and compliance:-| https://grpc.testnet.xmtp.network | US-EAST-2 | ... -| https://grpc2.testnet.xmtp.network | EU-NORTH-1 | ... +| [grpc.testnet.xmtp.network](https://grpc.testnet.xmtp.network) | US-EAST-2 | ... +| [grpc2.testnet.xmtp.network](https://grpc2.testnet.xmtp.network) | EU-NORTH-1 | ...🧰 Tools
🪛 Gitleaks (8.21.2)
17-17: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.17.2)
17-17: Bare URL used
null(MD034, no-bare-urls)
18-18: Bare URL used
null(MD034, no-bare-urls)
49-52: Add AppChain environment variables
With the new multi-chain support, we should document the correspondingXMTPD_APP_CHAIN_*settings alongside the settlement-chain vars. For example:+export XMTPD_APP_CHAIN_RPC_URL="http://localhost:7546/" +export XMTPD_APP_CHAIN_CHAIN_ID=31338This ensures users can configure both chains explicitly.
50-56: Namespace environment variables consistently
The variablesADMIN_PRIVATE_KEY,NODE_HTTP_ADDRESS,NODE_OWNER_ADDRESS, andNODE_SIGNING_KEY_PUBlack theXMTPD_prefix used elsewhere. To avoid collisions and improve clarity, consider renaming them:-export ADMIN_PRIVATE_KEY="0xDEADBEEF" +export XMTPD_ADMIN_PRIVATE_KEY="0xDEADBEEF" -export NODE_HTTP_ADDRESS="https://grpc.example.com" +export XMTPD_NODE_HTTP_ADDRESS="https://grpc.example.com" -export NODE_OWNER_ADDRESS="0xDEADBEEF" +export XMTPD_NODE_OWNER_ADDRESS="0xDEADBEEF" -export NODE_SIGNING_KEY_PUB="0xDEADBEEF" +export XMTPD_NODE_SIGNING_KEY_PUB="0xDEADBEEF"
85-104: Replace hard tabs with spaces in JSON example
The JSON code block uses hard tabs for indentation (lines 85–104), which can render inconsistently. Replace tabs with a consistent number of spaces (e.g., two spaces per level):- "level": "INFO", - "time": "2025-05-06T16:39:35.737+0200", - "message": "got nodes", + "level": "INFO", + "time": "2025-05-06T16:39:35.737+0200", + "message": "got nodes",🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
85-85: Hard tabs
Column: 1(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 1(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 1(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 1(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 1(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 1(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 1(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 1(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 1(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 1(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 1(MD010, no-hard-tabs)
99-99: Hard tabs
Column: 1(MD010, no-hard-tabs)
100-100: Hard tabs
Column: 1(MD010, no-hard-tabs)
101-101: Hard tabs
Column: 1(MD010, no-hard-tabs)
102-102: Hard tabs
Column: 1(MD010, no-hard-tabs)
103-103: Hard tabs
Column: 1(MD010, no-hard-tabs)
104-104: Hard tabs
Column: 1(MD010, no-hard-tabs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
dev/local.env(1 hunks)dev/up(1 hunks)doc/deploy.md(1 hunks)doc/onboarding.md(2 hunks)pkg/config/options.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- dev/up
🚧 Files skipped from review as they are similar to previous changes (3)
- doc/onboarding.md
- pkg/config/options.go
- dev/local.env
🧰 Additional context used
🪛 Gitleaks (8.21.2)
doc/deploy.md
17-17: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
31-31: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.17.2)
doc/deploy.md
17-17: Bare URL used
null
(MD034, no-bare-urls)
18-18: Bare URL used
null
(MD034, no-bare-urls)
85-85: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1
(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 1
(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 1
(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 1
(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 1
(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 1
(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 1
(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 1
(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 1
(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 1
(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 1
(MD010, no-hard-tabs)
99-99: Hard tabs
Column: 1
(MD010, no-hard-tabs)
100-100: Hard tabs
Column: 1
(MD010, no-hard-tabs)
101-101: Hard tabs
Column: 1
(MD010, no-hard-tabs)
102-102: Hard tabs
Column: 1
(MD010, no-hard-tabs)
103-103: Hard tabs
Column: 1
(MD010, no-hard-tabs)
104-104: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Upgrade Tests
- GitHub Check: Lint-Go
- GitHub Check: Test (Node)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Code Review
Restructure contract configuration to support separate application and settlement chain deploymentsThe changes introduce a new multi-chain architecture by restructuring the contract configuration into separate application chain and settlement chain components. Key changes include:
📍Where to StartStart with the configuration structure changes in options.go which defines the new multi-chain architecture, followed by validation.go which implements the validation logic for the new structure. Macroscope summarized 299498f. |
|
After merging this there's a need to:
|
mkysel
left a comment
mkysel
left a comment
There was a problem hiding this comment.
LGTM. Can we still accept the old XMTPD variables and deprecate them instead? It won't break anyone and it will be easier to deploy
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pkg/config/validation.go (2)
23-24: Error key inconsistency persists in validation messages.The error key
--contracts.app-chain.messages-addressmentioned in the past review comment has been changed to--contracts.app-chain.group-message-broadcaster-address, which is correct for the new nested structure. However, other inconsistencies with error key formatting remain throughout the validation code.
120-133: Inconsistent error key formatting needs to be addressed.The error keys for fields such as
DB.WriterConnectionString,payer.PrivateKey, andSigner.PrivateKeyuse a different format than the kebab-case style used for contract option fields. This inconsistency was noted in a previous review and should be standardized.Apply this diff to make error keys consistent:
- missingSet["--DB.WriterConnectionString"] = struct{}{} + missingSet["--db.writer-connection-string"] = struct{}{} - missingSet["--payer.PrivateKey"] = struct{}{} + missingSet["--payer.private-key"] = struct{}{} - missingSet["--Signer.PrivateKey"] = struct{}{} + missingSet["--signer.private-key"] = struct{}{}
🧹 Nitpick comments (1)
pkg/config/validation.go (1)
9-169: Consider adding a function comment to document in-place modification behavior.The function now modifies the input options directly, particularly when copying values from flat fields to the nested structure in single-chain mode. This behavior should be documented in a function comment.
Add a function comment like this:
+// ValidateServerOptions validates the ServerOptions and, for backward compatibility, +// copies values from legacy flat fields to the new nested AppChain and SettlementChain +// structure when in single-chain mode. func ValidateServerOptions(options *ServerOptions) error {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/replication/main.go(2 hunks)dev/local.env(1 hunks)pkg/config/options.go(2 hunks)pkg/config/validation.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/replication/main.go
- pkg/config/options.go
- dev/local.env
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/config/validation.go (1)
pkg/config/options.go (1)
ServerOptions(105-120)
🔇 Additional comments (4)
pkg/config/validation.go (4)
9-9: Function signature updated to allow in-place modification of options.The signature change from value to pointer parameter allows the function to modify the options in-place, particularly for copying values from legacy fields to the new nested structure. This is appropriate for enabling backward compatibility while supporting the new multi-chain configuration.
13-115: Multi-chain support structure looks good with proper validation.The code now correctly differentiates between multi-chain and single-chain deployments, with comprehensive validation for both scenarios. For backward compatibility, legacy field values are properly copied to the new nested structure.
171-174: Helper function effectively determines multi-chain deployment.The
isMultiChainDeploymentfunction provides a clear way to determine if a multi-chain deployment is configured by checking if both App Chain and Settlement Chain RPC URLs are specified. This is a good approach for detecting the deployment type.
117-117: Good separation of common validations with clear comment.Adding a comment to clearly separate common validations from chain-specific validations improves code readability and maintainability.
fbac
force-pushed
the
05-06-support_multi_chain
branch
2 times, most recently
from
963b25a to
7045ccc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/config/validation.go (3)
142-145: Consider enhancing multi-chain detection logicThe
isMultiChainDeploymentfunction currently only checks if both RPC URLs are set. You might want to consider additional conditions for more robust detection, such as verifying other required fields for multi-chain deployments are properly configured.func isMultiChainDeployment(options ServerOptions) bool { - return options.Contracts.AppChain.RpcURL != "" && - options.Contracts.SettlementChain.RpcURL != "" + return options.Contracts.AppChain.RpcURL != "" && + options.Contracts.SettlementChain.RpcURL != "" && + // Consider adding additional checks if appropriate + (options.Contracts.AppChain.GroupMessageBroadcasterAddress != "" || + options.Contracts.SettlementChain.NodeRegistryAddress != "") }
85-117: Reduce duplication in WriterConnectionString validationThe validation for
DB.WriterConnectionStringis repeated in multiple places. Consider refactoring to check this field once and then check which features are enabled.- if options.Payer.Enable { - validateField( - options.DB.WriterConnectionString, - "db.writer-connection-string", - missingSet, - ) - validateField(options.Payer.PrivateKey, "payer.private-key", missingSet) - } - - if options.Replication.Enable { - validateField( - options.DB.WriterConnectionString, - "db.writer-connection-string", - missingSet, - ) - validateField(options.Signer.PrivateKey, "signer.private-key", missingSet) - } - - if options.Sync.Enable { - validateField( - options.DB.WriterConnectionString, - "db.writer-connection-string", - missingSet, - ) - } - - if options.Indexer.Enable { - validateField( - options.DB.WriterConnectionString, - "db.writer-connection-string", - missingSet, - ) - } + // Check if any feature requiring DB writer connection is enabled + if options.Payer.Enable || options.Replication.Enable || options.Sync.Enable || options.Indexer.Enable { + validateField( + options.DB.WriterConnectionString, + "db.writer-connection-string", + missingSet, + ) + } + + // Check feature-specific required fields + if options.Payer.Enable { + validateField(options.Payer.PrivateKey, "payer.private-key", missingSet) + } + + if options.Replication.Enable { + validateField(options.Signer.PrivateKey, "signer.private-key", missingSet) + }
180-196: Enhance type safety in validateField functionThe current implementation doesn't handle unexpected types. Consider adding a default case to catch unexpected types that might be passed to this function in the future.
func validateField(value interface{}, fieldName string, set map[string]struct{}) { switch v := value.(type) { case string: if v == "" { set[fmt.Sprintf("--%s", fieldName)] = struct{}{} } case int: if v <= 0 { set[fmt.Sprintf("--%s must be greater than 0", fieldName)] = struct{}{} } case time.Duration: if v <= 0 { set[fmt.Sprintf("--%s must be greater than 0", fieldName)] = struct{}{} } + default: + // Log or handle unexpected types + set[fmt.Sprintf("--%s has unsupported type for validation", fieldName)] = struct{}{} } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/replication/main.go(2 hunks)dev/local.env(1 hunks)pkg/config/options.go(2 hunks)pkg/config/validation.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/replication/main.go
- dev/local.env
- pkg/config/options.go
🔇 Additional comments (4)
pkg/config/validation.go (4)
26-29: Correctly using struct field naming in error messagesThe error key now correctly references
group-message-broadcaster-addressinstead ofmessages-address, addressing the issue from previous reviews.
31-34: Correctly using struct field naming in error messagesThe error key now correctly references
identity-update-broadcaster-addressinstead of the previous incorrect naming, addressing the issue from previous reviews.
10-140: Well-structured validation logic for multi-chain supportThe refactored
ValidateServerOptionsfunction cleanly handles both multi-chain and single-chain deployments, with appropriate validation for each case. The use of a pointer parameter is also a good performance improvement.
147-178: Good backward compatibility with normalization functionThe
normalizeSingleChainConfigfunction provides excellent backward compatibility by copying values from deprecated fields to the new nested structure. This ensures a smooth transition for existing configurations.
fbac
force-pushed
the
05-06-support_multi_chain
branch
from
8bcb993 to
ef0e77b
Compare
### Restructure contract configuration to support separate application and settlement chain configurations in the blockchain client Reorganizes contract configuration by introducing a nested structure with `AppChain` and `SettlementChain` properties in [options.go](https://github.com/xmtp/xmtpd/pull/755/files#diff-6731fb6f709392ce3e37d3b0c42074cddbce566dad2bab86af24ba7585eeb57c). The changes include: * Creates new `AppChainOptions` and `SettlementChainOptions` structs to separate chain-specific configurations * Renames contract addresses to be more descriptive (e.g., `MessagesContractAddress` to `GroupMessageBroadcasterAddress`) * Updates all blockchain client components to use the new configuration structure * Modifies validation logic in [validation.go](https://github.com/xmtp/xmtpd/pull/755/files#diff-3e0b3707cc5d712d06d1eeb6a67c10b45a7ba0b27e972924d71df3c8e539f23b) to handle the new nested configuration format * Updates test utilities and configuration generators to support the new structure #### 📍Where to Start Start with the configuration structure changes in [options.go](https://github.com/xmtp/xmtpd/pull/755/files#diff-6731fb6f709392ce3e37d3b0c42074cddbce566dad2bab86af24ba7585eeb57c) which defines the new `AppChainOptions` and `SettlementChainOptions` structs that form the foundation for all other changes. ---- _[Macroscope](https://app.macroscope.com) summarized 384b3a8._ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Separated blockchain settings into distinct "AppChain" and "SettlementChain" configuration sections. - Reorganized contract addresses, chain IDs, RPC URLs, and refresh intervals under nested configuration fields. - Updated environment variables and scripts to support the new multi-chain configuration structure. - **Bug Fixes** - Enhanced validation and initialization to correctly handle the distinct AppChain and SettlementChain configurations. - **Documentation** - Revised onboarding and deployment guides to reflect local development environment setup and updated CLI usage. - **Tests** - Updated all tests to align with the new nested configuration format for blockchain settings. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Restructure contract configuration to support separate application and settlement chain configurations in the blockchain client
Reorganizes contract configuration by introducing a nested structure with
AppChainandSettlementChainproperties in options.go. The changes include:AppChainOptionsandSettlementChainOptionsstructs to separate chain-specific configurationsMessagesContractAddresstoGroupMessageBroadcasterAddress)📍Where to Start
Start with the configuration structure changes in options.go which defines the new
AppChainOptionsandSettlementChainOptionsstructs that form the foundation for all other changes.Macroscope summarized 384b3a8.
Summary by CodeRabbit