Load env config via JSON #855
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. |
WalkthroughThis change transitions contract configuration handling from environment variable-based and deprecated single-chain fields to a unified, JSON-based approach. It introduces JSON config parsing, updates CLI and Docker scripts to use config files, modifies test utilities to load addresses from JSON, and removes obsolete fields and logic. Structs and validation functions are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Config
participant FileSystem
CLI->>Config: ParseJSONConfig(&options.Contracts)
alt ConfigFilePath set
Config->>FileSystem: Read JSON config file
FileSystem-->>Config: JSON data
else ConfigJson set
Config->>Config: Use inline JSON string
end
Config->>Config: Unmarshal JSON into ChainConfig
Config->>Config: Merge ChainConfig into ContractsOptions
Config-->>CLI: Return error if any
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 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ 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 (
|
|
@fbac this will probably collide with a lot of your PRs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/config/validation.go (1)
129-159: LGTM! Thoughtful precedence logic with proper environment variable preservation.The function correctly implements the precedence hierarchy where environment variables take precedence over JSON configuration. The logic for handling zero values and empty strings is appropriate for the field types.
The special handling of ChainID
31337(Anvil/Hardhat default) is a thoughtful touch for development environments.Consider addressing the TODO comment.
The TODO comment on line 158 indicates incomplete implementation for payers and reports configuration. While this doesn't affect current functionality, it should be tracked for completion.
Would you like me to help generate the implementation for the payers and reports configuration, or should this be tracked as a separate issue?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
cmd/cli/main.go(1 hunks)cmd/prune/main.go(1 hunks)cmd/replication/main.go(1 hunks)dev/docker/docker-compose-register.yml(4 hunks)dev/environments/anvil.json(1 hunks)dev/local.env(0 hunks)dev/run(1 hunks)dev/run-2(1 hunks)pkg/blockchain/blockchainPublisher_test.go(1 hunks)pkg/blockchain/migrator/migrator_test.go(1 hunks)pkg/blockchain/ratesAdmin_test.go(1 hunks)pkg/blockchain/registryAdmin_test.go(1 hunks)pkg/config/chainConfig.go(1 hunks)pkg/config/options.go(1 hunks)pkg/config/pruneOptions.go(1 hunks)pkg/config/validation.go(3 hunks)pkg/indexer/app_chain/contracts/group_message_storer_test.go(1 hunks)pkg/indexer/app_chain/contracts/identity_update_storer_test.go(1 hunks)pkg/indexer/e2e_test.go(1 hunks)pkg/testutils/config.go(1 hunks)pkg/testutils/utils.go(1 hunks)pkg/upgrade/docker_utils_test.go(4 hunks)
💤 Files with no reviewable changes (1)
- dev/local.env
🧰 Additional context used
🧬 Code Graph Analysis (12)
pkg/blockchain/migrator/migrator_test.go (1)
pkg/testutils/config.go (1)
NewContractsOptions(25-59)
pkg/indexer/app_chain/contracts/group_message_storer_test.go (1)
pkg/testutils/config.go (1)
NewContractsOptions(25-59)
pkg/indexer/app_chain/contracts/identity_update_storer_test.go (1)
pkg/testutils/config.go (1)
NewContractsOptions(25-59)
pkg/indexer/e2e_test.go (1)
pkg/testutils/config.go (1)
NewContractsOptions(25-59)
pkg/blockchain/blockchainPublisher_test.go (1)
pkg/testutils/config.go (1)
NewContractsOptions(25-59)
pkg/blockchain/registryAdmin_test.go (1)
pkg/testutils/config.go (1)
NewContractsOptions(25-59)
cmd/cli/main.go (1)
pkg/config/validation.go (1)
ParseJSONConfig(88-127)
pkg/blockchain/ratesAdmin_test.go (1)
pkg/testutils/config.go (1)
NewContractsOptions(25-59)
pkg/upgrade/docker_utils_test.go (1)
pkg/testutils/utils.go (1)
GetScriptPath(8-13)
pkg/config/pruneOptions.go (1)
pkg/config/options.go (4)
DbOptions(40-48)LogOptions(89-92)SignerOptions(94-96)SettlementChainOptions(30-38)
pkg/testutils/config.go (3)
pkg/config/options.go (3)
ContractsOptions(12-20)AppChainOptions(22-28)SettlementChainOptions(30-38)pkg/testutils/utils.go (1)
GetScriptPath(8-13)pkg/config/chainConfig.go (1)
ChainConfig(3-16)
pkg/config/chainConfig.go (3)
pkg/abi/payerregistry/PayerRegistry.go (1)
PayerRegistry(48-52)pkg/abi/rateregistry/RateRegistry.go (1)
RateRegistry(51-55)pkg/abi/settlementchainparameterregistry/SettlementChainParameterRegistry.go (1)
SettlementChainParameterRegistry(42-46)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test (Node)
- GitHub Check: Upgrade Tests
🔇 Additional comments (32)
pkg/testutils/utils.go (1)
8-13: LGTM! Clean utility function for test path resolution.The implementation correctly uses
runtime.Caller(1)to get the caller's file path and joins it with additional path elements. This is a common and reliable pattern for test utilities that need to locate files relative to the test source.pkg/config/pruneOptions.go (1)
11-11: Correct type change aligning with multi-chain configuration refactor.The change from
NodesContractOptiontoSettlementChainOptionsproperly aligns the prune configuration with the new multi-chain contract configuration approach. This enables JSON-based configuration loading and provides more comprehensive settlement chain options.cmd/prune/main.go (1)
55-55: Correct field name update consistent with configuration refactor.The change from
NodesContractAddresstoNodeRegistryAddressproperly aligns with the updatedSettlementChainOptionsstruct. The field serves the same purpose for namespace building while using the new multi-chain configuration structure.cmd/replication/main.go (1)
89-89: Correct field path update for multi-chain configuration.The change from
options.Contracts.NodesContract.NodesContractAddresstooptions.Contracts.SettlementChain.NodeRegistryAddressproperly migrates from the deprecated nested contract structure to the new settlement chain configuration. This aligns with the architectural shift to JSON-based multi-chain configuration management.dev/run (1)
14-14:Details
✅ Verification successful
LGTM! Environment variable for JSON config file path added correctly.
The addition of
XMTPD_CONTRACTS_CONFIG_FILE_PATHenvironment variable aligns with the PR objective of transitioning to JSON-based contract configuration. The relative path./dev/environments/anvil.jsonshould work when the script is executed from the repository root.Let's verify that the JSON config file exists at the specified path:
🏁 Script executed:
#!/bin/bash # Description: Verify the anvil.json config file exists and is valid JSON # Check if the config file exists if [ -f "./dev/environments/anvil.json" ]; then echo "✓ Config file exists" # Validate JSON syntax if jq empty "./dev/environments/anvil.json" 2>/dev/null; then echo "✓ Valid JSON syntax" # Show the structure echo "Config file structure:" jq . "./dev/environments/anvil.json" else echo "✗ Invalid JSON syntax" exit 1 fi else echo "✗ Config file not found at ./dev/environments/anvil.json" exit 1 fiLength of output: 972
✔️ Verified XMTPD_CONTRACTS_CONFIG_FILE_PATH
– Confirmed
./dev/environments/anvil.jsonexists
– Valid JSON syntax and expected structureNo further changes needed.
pkg/blockchain/migrator/migrator_test.go (1)
26-26: Function signature updated correctly to match the new JSON config approach.The addition of the testing parameter
tas the first argument totestutils.NewContractsOptionsis correct and necessary. The updated function signature inpkg/testutils/config.gonow requires this parameter for test assertions when loading and parsing the JSON configuration file.pkg/blockchain/ratesAdmin_test.go (1)
19-19: Function signature updated correctly for JSON config support.The addition of the testing parameter
ttotestutils.NewContractsOptionsis consistent with the broader refactor to JSON-based configuration. This matches the updated function signature that requires the testing context for proper error handling during JSON config file loading.pkg/indexer/e2e_test.go (1)
33-33: Function signature consistently updated across all test files.The addition of the testing parameter
ttotestutils.NewContractsOptionscompletes the consistent update across all test files. This change aligns with the new JSON-based configuration approach where the function requires a testing context for proper error handling and assertions during config file loading.pkg/indexer/app_chain/contracts/group_message_storer_test.go (1)
27-27: LGTM: Correct parameter update for new function signature.The change correctly adds the testing context parameter to match the updated
NewContractsOptionsfunction signature. This enables the function to userequire.NoErrorfor test assertions when reading JSON configuration.pkg/blockchain/blockchainPublisher_test.go (1)
20-20: LGTM: Consistent parameter update for new function signature.The change correctly adds the testing context parameter to align with the updated
NewContractsOptionsfunction signature, consistent with other test file updates in this PR.cmd/cli/main.go (1)
137-139: LGTM: Proper JSON config parsing integration.The JSON config parsing is correctly integrated into the CLI initialization flow. The placement after command validation and before CLI struct creation is logical, and the error handling follows the established pattern in this function.
pkg/blockchain/registryAdmin_test.go (1)
21-21: LGTM: Consistent parameter update for new function signature.The change correctly adds the testing context parameter to match the updated
NewContractsOptionsfunction signature, maintaining consistency with the other test file updates in this PR.pkg/indexer/app_chain/contracts/identity_update_storer_test.go (1)
31-31: LGTM! Function call updated correctly for new signature.The addition of the test context
tas the first parameter correctly aligns with the updatedtestutils.NewContractsOptionssignature that now loads contract configuration from JSON files instead of using hardcoded values.dev/run-2 (2)
11-11: LGTM! Reflection feature enabled.The addition of
XMTPD_REFLECTION_ENABLE=trueis straightforward and properly enables the reflection feature.
16-16:Details
✅ Verification successful
Verify the JSON config file path is correct.
The environment variable correctly points to the new JSON configuration file. Ensure that the relative path
./dev/environments/anvil.jsonresolves correctly when this script is executed from the expected working directory.
🏁 Script executed:
#!/bin/bash # Verify that the anvil.json file exists at the specified path if [ -f "./dev/environments/anvil.json" ]; then echo "✓ anvil.json file exists at the specified path" cat ./dev/environments/anvil.json else echo "✗ anvil.json file not found at ./dev/environments/anvil.json" echo "Looking for anvil.json files in the repository:" fd "anvil.json" fiLength of output: 878
Config file path verified
The
anvil.jsonfile exists at./dev/environments/anvil.jsonand contains the expected configuration. No further action needed.dev/docker/docker-compose-register.yml (4)
13-15: LGTM! JSON config properly mounted and configured for register-node-1.The volume mount correctly maps the host file to the container, and the CLI argument properly references the mounted config file path.
Also applies to: 22-22
32-34: LGTM! JSON config properly mounted and configured for enable-node-1.Consistent implementation with the register-node-1 service for volume mounting and CLI argument configuration.
Also applies to: 39-39
52-54: LGTM! JSON config properly mounted and configured for register-node-2.Consistent implementation across all services for the JSON configuration approach.
Also applies to: 61-61
74-76: LGTM! JSON config properly mounted and configured for enable-node-2.All services now consistently use the JSON-based contract configuration, completing the transition from environment variable-based config.
Also applies to: 81-81
dev/environments/anvil.json (1)
1-14:Details
❓ Verification inconclusive
Well-structured JSON configuration file for contract addresses.
The JSON structure properly defines chain configuration and contract addresses for both app chain and settlement chain. The use of chain ID 31337 is appropriate for local development environments.
Consider verifying that the contract addresses correspond to the correct deployed contracts in your development environment:
🏁 Script executed:
#!/bin/bash # Verify the JSON structure and validate Ethereum addresses echo "Validating JSON structure..." jq empty ./dev/environments/anvil.json && echo "✓ Valid JSON structure" || echo "✗ Invalid JSON" echo -e "\nContract addresses in config:" jq -r 'to_entries | map(select(.key | endswith("Manager") or endswith("Broadcaster") or endswith("Registry"))) | .[] | "\(.key): \(.value)"' ./dev/environments/anvil.json echo -e "\nVerifying address format (should all be 42 characters starting with 0x):" jq -r 'to_entries | map(select(.key | endswith("Manager") or endswith("Broadcaster") or endswith("Registry"))) | .[] | .value' ./dev/environments/anvil.json | while read addr; do if [[ $addr =~ ^0x[a-fA-F0-9]{40}$ ]]; then echo "✓ $addr" else echo "✗ $addr (invalid format)" fi doneLength of output: 2736
JSON configuration and address format validated
✓ The JSON structure is valid.
✓ All contract addresses conform to the
0x-prefixed, 40-hex-character format.Please verify that each address corresponds to the correct deployed contract on your local Anvil chain. For example:
curl -X POST http://localhost:8545 \ -H "Content-Type: application/json" \ -d '{"jsonrpc":"2.0","method":"eth_getCode","params":["<address>","latest"],"id":1}'Replace
<address>with each contract address and confirm the returned code is non-zero.pkg/upgrade/docker_utils_test.go (3)
37-37: Good refactoring to use centralized path utility.Replacing the local
getScriptPathfunction withtestutils.GetScriptPathimproves code reuse and consistency across the codebase.
91-91: Consistent use of centralized path utility.Good to see consistent application of the centralized path resolution utility here as well.
150-162:Details
✅ Verification successful
Verify JSON config file path and mounting configuration.
The container setup correctly adds the JSON config file path environment variable and mounts the anvil.json file. However, ensure the mounted file path is accessible and the file exists at the expected location.
🏁 Script executed:
#!/bin/bash # Description: Verify the anvil.json config file exists at the expected path # Expected: The file should exist and be readable # Check if the anvil.json file exists fd "anvil.json" --type f # Verify the file content structure if [ -f "dev/environments/anvil.json" ]; then echo "=== anvil.json content ===" cat dev/environments/anvil.json echo "" echo "=== JSON validation ===" jq . dev/environments/anvil.json > /dev/null && echo "Valid JSON" || echo "Invalid JSON" else echo "anvil.json file not found at expected location" fiLength of output: 1020
✅ Verified JSON config mount and path
The file
dev/environments/anvil.jsonexists, is readable, and contains valid JSON. Mounting it to/cfg/anvil.jsonand settingXMTPD_CONTRACTS_CONFIG_FILE_PATHis correct—no further action required.pkg/testutils/config.go (4)
20-23: Well-defined constants for configuration paths.The constants clearly define the file depth and JSON config path, making the code more maintainable and the file location explicit.
25-25: Function signature change improves testability.Adding the
*testing.Tparameter allows for proper error assertions usingrequire.NoError, which is better than silent failures in test contexts.
26-38: Solid JSON config file handling with proper error management.The implementation correctly:
- Opens the file using the centralized path utility
- Uses
require.NoErrorfor test-appropriate error handling- Properly closes the file with defer
- Reads and unmarshals JSON with error checking
The resource management is sound and error handling is appropriate for test utilities.
45-46: Contract addresses properly sourced from JSON config.The contract addresses are now correctly loaded from the
chainConfigstruct instead of hardcoded constants, supporting the flexible JSON-based configuration approach.Also applies to: 53-55
pkg/config/chainConfig.go (1)
3-16: Well-structured configuration schema with comprehensive fields.The
ChainConfigstruct effectively covers all necessary blockchain configuration parameters:
- Deployment blocks for both chains
- Chain IDs for proper network identification
- All essential contract addresses for the multi-chain setup
- Consistent JSON tag naming using camelCase
The struct provides a clean schema for the new JSON-based configuration system.
pkg/config/options.go (1)
18-19: Appropriate JSON configuration options added.The new fields provide flexible configuration options:
ConfigFilePath: Allows specifying a JSON config file path via CLI or environment variableConfigJson: Enables direct JSON config input, useful for inline configurationBoth fields have:
- Proper CLI long option names following kebab-case convention
- Appropriate environment variable names with
XMTPD_CONTRACTS_prefix- Clear, descriptive help text
This design supports both file-based and direct JSON configuration patterns.
pkg/config/validation.go (3)
4-4: LGTM! Appropriate imports for JSON configuration.The added standard library imports are necessary and appropriate for the new JSON-based configuration functionality.
Also applies to: 7-8
19-22: LGTM! Clean integration with proper error handling.The early placement of JSON config parsing in the validation flow follows a good fail-fast pattern, and error handling is properly implemented.
88-127: LGTM! Well-implemented JSON configuration parsing.The function demonstrates good Go practices:
- Proper validation preventing conflicting config sources
- Correct resource management with deferred file cleanup
- Consistent error handling throughout
- DRY principle by reusing
fillConfigFromJsonfor both file and string inputsThe implementation handles both configuration sources appropriately and follows standard Go file operation patterns.
Replace environment variable-based contract configuration with JSON file configuration across CLI commands, Docker setup, and test utilitiesThis PR introduces JSON-based configuration for blockchain contracts to replace environment variable configuration. The changes include: • Added new 📍Where to StartStart with the Changes since #855 opened
Macroscope summarized 6cf2a32. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| if options.AppChain.IdentityUpdateBroadcasterAddress == "" { | ||
| options.AppChain.IdentityUpdateBroadcasterAddress = config.IdentityUpdateBroadcaster | ||
| } | ||
| if options.AppChain.ChainID == 0 || options.AppChain.ChainID == 31337 { |
There was a problem hiding this comment.
The checks in fillConfigFromJson for options.AppChain.ChainID and options.SettlementChain.ChainID treat the default value (31337) the same as an unset value, causing an explicit environment setting of 31337 to be overwritten by the JSON config. Consider only overriding the chain ID when it’s truly unset (i.e., zero) so that explicit flags or environment variables—even if they match the default—take full precedence over the JSON settings.
- if options.AppChain.ChainID == 0 || options.AppChain.ChainID == 31337 {
+ if options.AppChain.ChainID == 0 {
- if options.SettlementChain.ChainID == 0 || options.SettlementChain.ChainID == 31337 {
+ if options.SettlementChain.ChainID == 0 {🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
There was a problem hiding this comment.
31337 is set as the default, as such if the user does not specify anything, it will get set
There was a problem hiding this comment.
I understand 31337 is the default, but the issue is about precedence: if a user explicitly sets XMTPD_APP_CHAIN_CHAIN_ID=31337 in their environment, that explicit setting should take precedence over JSON config. Currently, the || options.AppChain.ChainID == 31337 logic treats an explicit environment setting of 31337 the same as no setting at all, causing the JSON to override the user's explicit choice.
Replace individual contract address environment variables with centralized JSON configuration loading in CLI, prune, and replication commands
The system now loads contract addresses and chain configuration from a centralized JSON file instead of individual environment variables. Key changes include:
• Added
config.ParseJSONConfig()function that loads contract configuration from either a JSON file path or JSON string specified in environment variables• Created new
ChainConfigstruct in pkg/config/chainConfig.go defining the JSON schema for contract addresses and chain IDs• Modified
ContractsOptionsstruct to includeConfigFilePathandConfigJsonfields for JSON-based configuration loading• Updated CLI commands in cmd/cli/main.go, cmd/prune/main.go, and cmd/replication/main.go to use new contract address field names
• Removed all individual contract address environment variables from dev/local.env and replaced with
XMTPD_CONTRACTS_CONFIG_FILE_PATHpointing to dev/environments/anvil.json• Updated Docker Compose configuration in dev/docker/docker-compose-register.yml to mount the JSON configuration file and pass
--contracts.config-file-pathargument• Modified test utilities in pkg/testutils/config.go to load contract addresses from JSON configuration instead of hardcoded constants
📍Where to Start
Start with the
ParseJSONConfigfunction in pkg/config/validation.go to understand how JSON configuration is loaded and parsed, then examine how it's called in cmd/cli/main.go.Macroscope summarized 9f42b0e.
Summary by CodeRabbit
New Features
Bug Fixes
Chores