Test utility to start an anvil instance #629
Conversation
WalkthroughA new testing utility package named Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test
participant S as StartAnvil
participant F as findFreePort
participant C as AnvilProcess
participant W as waitForAnvil
participant BC as BlockchainClient
T->>S: Call StartAnvil(showLogs)
S->>F: findFreePort()
F-->>S: Return free port
S->>C: Start Anvil process on free port
S->>W: waitForAnvil(url)
W->>BC: Request chain ID
BC-->>W: Return chain ID or error
W-->>S: Confirm Anvil ready
S-->>T: Return URL and cleanup function
sequenceDiagram
participant T as Test
participant CR as ConcurrentRunner
participant A1 as AnvilInstance1
participant A2 as AnvilInstance2
note over CR: ... up to 10 instances
T->>CR: Start concurrent initialization (goroutines)
CR->>A1: StartAnvil (instance 1)
CR->>A2: StartAnvil (instance 2)
A1-->>CR: Return URL & cleanup function
A2-->>CR: Return URL & cleanup function
note over T,CR: Validate each instance via blockchain client queries
Possibly related PRs
Suggested reviewers
Suggested labels
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24) Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
neekolas
force-pushed
the
03-13-test_utility_to_start_an_anvil_instance
branch
from
8e49750 to
353362d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (5)
pkg/testutils/anvil/anvil_test.go (2)
34-40: Consider adding context with timeout for goroutinesThe goroutines launching Anvil instances don't have a timeout mechanism. If an instance fails to start, the test could hang indefinitely.
// Launch goroutines to start anvil instances for i := 0; i < numInstances; i++ { go func() { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() url, cleanup := StartAnvil(t, false) results <- anvilInstance{url: url, cleanup: cleanup} }() }
47-49: Potential nil dereference risk in cleanup functionThis check is reasonable, but consider adding a log message if
cleanupis nil as it would indicate an unexpected state.if instance.cleanup != nil { defer instance.cleanup() + } else { + t.Logf("Warning: nil cleanup function for instance %d", i) }pkg/testutils/anvil/anvil.go (3)
26-54: Remove unreachable code after t.FatalfThe
returnstatement aftert.Fatalfwill never be executed sincet.Fatalfterminates the test.case <-ctx.Done(): t.Fatalf("Timed out waiting for Anvil to start: %v", ctx.Err()) - return default:
40-52: Consider adding exponential backoff for pollingThe current implementation uses a fixed 100ms sleep between retries. An exponential backoff strategy might be more efficient for handling varying startup times.
+ // Calculate backoff time (starts at 100ms, doubles each retry up to 1s) + backoff := 100 * time.Millisecond + // Use backoff time for sleep, doubling it each retry up to 1 second + if backoff < time.Second { + backoff *= 2 + } - time.Sleep(100 * time.Millisecond) + time.Sleep(backoff)
72-75: Error handling in cleanup functionThe cleanup function silently ignores errors. Consider logging errors for better debugging, especially in test environments.
return url, func() { - _ = cmd.Process.Kill() - _ = cmd.Wait() + if err := cmd.Process.Kill(); err != nil { + fmt.Fprintf(os.Stderr, "Error killing Anvil process: %v\n", err) + } + if err := cmd.Wait(); err != nil && !isKilledProcessError(err) { + fmt.Fprintf(os.Stderr, "Error waiting for Anvil process to exit: %v\n", err) + } }You would also need to add a helper function to check if the error is due to the process being killed:
func isKilledProcessError(err error) bool { // Different OS may return different errors when a process is killed return strings.Contains(err.Error(), "signal: killed") || strings.Contains(err.Error(), "process already finished") }
🛑 Comments failed to post (3)
pkg/testutils/anvil/anvil_test.go (1)
1-1:
⚠️ Potential issuePackage name mismatch causing build failure
The package name should be
anvilto match the implementation file, nottestutils. This mismatch is causing the pipeline failure.-package testutils +package anvil📝 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.package anvil🧰 Tools
🪛 GitHub Actions: Lint
[error] 1-1: found packages anvil (anvil.go) and testutils (anvil_test.go) in pkg/testutils/anvil (typecheck)
pkg/testutils/anvil/anvil.go (2)
60-60: 💡 Verification agent
🧩 Analysis chain
Verify anvil binary availability
The code assumes that the
anvilbinary is available in the PATH. Consider adding a check at startup or documenting this requirement.
🏁 Script executed:
#!/bin/bash # Check if anvil is installed and available in PATH which anvil >/dev/null 2>&1 if [ $? -ne 0 ]; then echo "❌ 'anvil' binary not found in PATH. Make sure it's installed." exit 1 else echo "✅ 'anvil' binary found in PATH: $(which anvil)" fiLength of output: 184
Action Required: Implement a Runtime Check or Document the 'anvil' Binary Requirement
The test script confirmed that the
anvilbinary is missing from the PATH on your system. Since the current code assumes its presence (as seen in theexec.Command("anvil", "--port", …)call), you should either add a runtime check to verify its availability—using something like Go’sexec.LookPath("anvil")—or explicitly document this dependency so that users know to install it beforehand.
- Suggestion:
- Runtime Check: Before invoking the command, check with:
if _, err := exec.LookPath("anvil"); err != nil { log.Fatalf("anvil binary not found in PATH; please ensure it is installed") }- Documentation: If you prefer not to add a runtime check, update your README or startup instructions to state that the
anvilbinary must be available in the PATH.
1-1:
⚠️ Potential issuePackage name mismatch causing build failure
The package should be named
testutilsto align with the directory structure and avoid conflicts with the test file. This inconsistency is causing the pipeline failure.-package anvil +package testutilsAlternatively, if you prefer to keep this package name as
anvil, then you should update the test file to match:// In anvil_test.go -package testutils +package anvil📝 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.package testutils🧰 Tools
🪛 GitHub Check: Lint-Go
[failure] 1-1:
: found packages anvil (anvil.go) and testutils (anvil_test.go) in pkg/testutils/anvil (typecheck)🪛 GitHub Actions: Lint
[error] 1-1: found packages anvil (anvil.go) and testutils (anvil_test.go) in pkg/testutils/anvil (typecheck)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/testutils/anvil/anvil_test.go (1)
47-49: Consider adding error handling for nil cleanup functionsWhile the current check prevents calling nil cleanup functions, consider logging a warning when this condition occurs as it may indicate an unexpected state.
if instance.cleanup != nil { defer instance.cleanup() +} else { + t.Logf("Warning: nil cleanup function for instance with URL %s", instance.url) }pkg/testutils/anvil/anvil.go (2)
26-54: Consider implementing exponential backoff for more efficient pollingWhile the current fixed interval polling works, implementing exponential backoff would be more efficient for handling varying startup times.
- // Wait a bit before trying again - time.Sleep(100 * time.Millisecond) + // Use exponential backoff + waitTime := time.Duration(attempt*50) * time.Millisecond + if waitTime > 500*time.Millisecond { + waitTime = 500 * time.Millisecond + } + time.Sleep(waitTime)You would need to add an
attemptcounter to track the number of retries.
72-75: Improve error handling in cleanup functionThe current cleanup function ignores errors from
Kill()andWait(). Consider logging these errors to help troubleshoot issues during test cleanup.return url, func() { - _ = cmd.Process.Kill() - _ = cmd.Wait() + if err := cmd.Process.Kill(); err != nil { + t.Logf("Error killing anvil process: %v", err) + } + if err := cmd.Wait(); err != nil && !isExpectedKillError(err) { + t.Logf("Error waiting for anvil process to exit: %v", err) + } } // Helper function to determine if an error is expected after killing the process func isExpectedKillError(err error) bool { return err.Error() == "signal: killed" || err.Error() == "process already finished" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/testutils/anvil/anvil.go(1 hunks)pkg/testutils/anvil/anvil_test.go(1 hunks)
🔇 Additional comments (3)
pkg/testutils/anvil/anvil_test.go (2)
11-21: LGTM: Well-structured test for basic Anvil startupThe test effectively verifies both the startup and connectivity of an Anvil instance by checking for successful connection and chain ID retrieval.
23-65: Effective concurrent testing approachThis test properly validates the concurrent operation of multiple Anvil instances, which is important for ensuring thread safety in test environments. The use of channels, goroutines, and deferred cleanup functions is well structured.
pkg/testutils/anvil/anvil.go (1)
17-24: LGTM: Effective port allocationThe function correctly finds an available TCP port by letting the system assign one, then retrieving that assigned port number.
neekolas
force-pushed
the
03-13-test_utility_to_start_an_anvil_instance
branch
from
353362d to
18c0a33
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/testutils/anvil/anvil_test.go (4)
51-57: Consider adding context with timeout for goroutinesWhile the concurrent startup approach is good, consider adding a context with timeout to prevent potential hanging if an Anvil instance fails to start properly.
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() go func() { - url, cleanup := StartAnvil(t, false) + url, cleanup := StartAnvil(t, false) // Consider passing ctx if StartAnvil supports it results <- anvilInstance{url: url, cleanup: cleanup} }()
59-67: Ensure proper cleanup in failure scenariosThe current implementation defers cleanup for each instance, but if a test assertion fails before all instances are collected, some goroutines might be left running. Consider ensuring cleanup happens even if the test fails partway through.
// Collect all instances instances := make([]anvilInstance, 0, numInstances) + defer func() { + // Clean up any collected instances in case of early test failure + for _, instance := range instances { + if instance.cleanup != nil { + instance.cleanup() + } + } + }() for range numInstances { instance := <-results instances = append(instances, instance) - if instance.cleanup != nil { - defer instance.cleanup() - } }
61-63: Add timeout to channel receive operationsTo prevent the test from hanging if some goroutines fail to send results, consider adding a timeout to the channel receive operations.
for range numInstances { - instance := <-results + select { + case instance := <-results: + instances = append(instances, instance) + case <-time.After(10 * time.Second): + t.Fatalf("Timeout waiting for anvil instance to start") + } - instances = append(instances, instance) }
3-9: Consider adding timeout-related importsIf you implement the suggested timeout improvements, you'll need to add the time package to your imports.
import ( "context" "testing" + "time" "github.com/stretchr/testify/require" "github.com/xmtp/xmtpd/pkg/blockchain" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/testutils/anvil/anvil.go(1 hunks)pkg/testutils/anvil/anvil_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/testutils/anvil/anvil.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Test (Node)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (5)
pkg/testutils/anvil/anvil_test.go (5)
11-21: Good test structure for verifying basic Anvil startupThis test correctly validates that the Anvil instance starts and can be connected to via a blockchain client. The use of defer for cleanup is a good practice that ensures resources are released even if the test fails.
23-38: Good validation of cleanup functionalityThis test effectively verifies that the cleanup function properly terminates the Anvil instance by checking that subsequent blockchain operations fail with an error. This is a comprehensive approach to testing the cleanup functionality.
44-49: Well-structured type for test coordinationThe
anvilInstancestruct provides a clean way to organize the URL and cleanup function for each test instance.
71-82: Good detailed verification of each instanceThe test properly verifies each Anvil instance with detailed error messages that include the instance index. This will make debugging failures much easier.
40-83: Comprehensive test for concurrent Anvil startupThis test effectively validates that multiple Anvil instances can be started concurrently without conflicts. The approach of using goroutines and channels is appropriate for this use case, and the verification steps ensure each instance is fully functional.
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit (edited by Mike) - **New Features** - Added new configuration options for fuzz testing and formatting. - Introduced new functions for setting minimum and maximum payload sizes in messaging and identity updates. - **Refactor** - Streamlined contract structures and deployment paths across messaging, identity, node, and rates management systems. - Enhanced error handling and clarity in contracts and tests. - Reorganized import statements and inheritance structures for improved readability and maintainability. - Refactored for pure unit tests using harnesses for low-level setters and getters. - **Tests** - Expanded test coverage with consistent naming conventions and fuzz testing for more robust validation. - Improved testing framework for node management and rates management functionalities. - Enhanced error handling in tests to ensure proper validation of access controls and input conditions. - **Chore** - Consolidated internal setup and tooling, improving overall system clarity. - **Bug Fixes** - Corrected issues with visibility and inheritance in several contracts to ensure proper functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Far from complete, but a step in the right direction. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced enhanced performance tracking for key operations, enabling more accurate measurement of processing times, retry counts, and activity volumes. - Improved observability across publishing workflows and nonce management for deeper insights into system performance. - **Chores** - Expanded metrics collection to support proactive diagnostics and overall system reliability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Closes #486 - Simplify upgrade test logic by using testcontainers. - Continue on error in upgrade tests, instead make a comment with test results. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new GitHub Actions workflow for automated upgrade testing. - **Chores** - Updated multiple dependencies to boost stability, performance, and security. - **Tests** - Enhanced functionality to pull Docker images directly in tests. - Streamlined the upgrade test setup, including a dedicated test validating the latest development version. - Improved error handling and context management in test functions. - Refined container-based tests for improved reliability and clarity. - Improved version management in upgrade tests for better structure and clarity. - Reordered import statements in test files for better organization. - Removed the "Run Upgrade Tests" step from the GitHub Actions workflow. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling during event processing to ensure continuity even if blockchain queries fail, preventing unnecessary interruptions in operations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
### TL;DR Refactored blockchain tests to use Anvil for isolated test environments ## Issues #630 ### What changed? - Modified blockchain test files to use isolated Anvil instances for each test - Updated contract deployment functions to accept an RPC URL parameter - Added proper cleanup of Anvil instances after tests complete - Added a `Contract()` accessor method to `RatesAdmin` to expose the underlying contract - Moved blockchain package tests to `blockchain_test` package for better isolation - Replaced `defer cleanup()` with `t.Cleanup(cleanup)` in some tests - Added `NewContractsOptions` function that accepts an RPC URL parameter - Fixed initialization of deployed contracts in test utilities ### How to test? Run the blockchain tests: ``` go test ./pkg/blockchain/... -v ``` ### Why make this change? This change improves test reliability by ensuring each test runs in an isolated blockchain environment. By using separate Anvil instances for each test, we eliminate potential interference between tests and make them more deterministic. The code also properly cleans up resources after tests complete, preventing resource leaks. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new administrative interface that enables direct access to rate management functionality. - **Quality Improvements** - Upgraded blockchain integration and configuration to support dynamic contract deployments. - Streamlined resource cleanup and testing processes to enhance system stability and reliability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Refactored blockchain tests to use Anvil for isolated test environments #630 - Modified blockchain test files to use isolated Anvil instances for each test - Updated contract deployment functions to accept an RPC URL parameter - Added proper cleanup of Anvil instances after tests complete - Added a `Contract()` accessor method to `RatesAdmin` to expose the underlying contract - Moved blockchain package tests to `blockchain_test` package for better isolation - Replaced `defer cleanup()` with `t.Cleanup(cleanup)` in some tests - Added `NewContractsOptions` function that accepts an RPC URL parameter - Fixed initialization of deployed contracts in test utilities Run the blockchain tests: ``` go test ./pkg/blockchain/... -v ``` This change improves test reliability by ensuring each test runs in an isolated blockchain environment. By using separate Anvil instances for each test, we eliminate potential interference between tests and make them more deterministic. The code also properly cleans up resources after tests complete, preventing resource leaks. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Introduced a new administrative interface that enables direct access to rate management functionality. - **Quality Improvements** - Upgraded blockchain integration and configuration to support dynamic contract deployments. - Streamlined resource cleanup and testing processes to enhance system stability and reliability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…b.com:xmtp/xmtpd into 03-13-test_utility_to_start_an_anvil_instance
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
pkg/testutils/config.go (1)
10-10: 💡 Verification agent🧩 Analysis chain
Potentially unused constant.
The constant
BLOCKCHAIN_RPC_URLappears to be no longer used after the introduction of the Anvil utility, as RPC URLs are now dynamically generated.
🏁 Script executed:
#!/bin/bash # Check if BLOCKCHAIN_RPC_URL is used elsewhere in the codebase echo "Checking for usage of BLOCKCHAIN_RPC_URL..." rg "BLOCKCHAIN_RPC_URL" --type goLength of output: 205
Action Required: Remove the Unused Constant
Based on the search results, the constant
BLOCKCHAIN_RPC_URLis only defined inpkg/testutils/config.goand isn’t referenced anywhere else. This aligns with the switch to dynamic RPC URL generation using Anvil.
- Recommendation: Remove the constant if it’s no longer needed. If it should be retained for potential fallback or documentation purposes, please add a clarifying comment to justify its presence.
🧹 Nitpick comments (2)
pkg/testutils/contracts_test.go (1)
12-14: Well-structured Anvil usage with proper cleanup.Good implementation of starting an Anvil instance with proper cleanup using defer. The false parameter likely controls logging verbosity, though a comment explaining this would be helpful.
Consider adding a brief comment explaining what the
falseparameter toStartAnvilcontrols:- rpcUrl, cleanup := anvil.StartAnvil(t, false) + // false parameter disables verbose logging + rpcUrl, cleanup := anvil.StartAnvil(t, false)pkg/blockchain/blockchainPublisher_test.go (1)
50-54: Resource cleanup could be more explicit.While
defer cleanup()ensures Anvil will be cleaned up, it might be clearer to structure the cleanup sequence explicitly to ensure the Anvil instance is cleaned up before other resources.Consider restructuring the cleanup function for more explicit ordering:
return publisher, func() { - defer cleanup() cancel() client.Close() + cleanup() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
pkg/blockchain/blockchainPublisher_test.go(3 hunks)pkg/blockchain/migrator/migrator_test.go(3 hunks)pkg/blockchain/ratesAdmin.go(1 hunks)pkg/blockchain/ratesAdmin_test.go(2 hunks)pkg/blockchain/registryAdmin_test.go(1 hunks)pkg/blockchain/registryCaller_test.go(1 hunks)pkg/blockchain/rpcLogStreamer_test.go(2 hunks)pkg/indexer/e2e_test.go(3 hunks)pkg/indexer/storer/groupMessage_test.go(2 hunks)pkg/indexer/storer/identityUpdate_test.go(3 hunks)pkg/testutils/config.go(1 hunks)pkg/testutils/contracts.go(4 hunks)pkg/testutils/contracts_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/blockchain/registryCaller_test.go
🔇 Additional comments (42)
pkg/testutils/contracts.go (5)
107-107: Improved method signature with dynamic RPC URLThe updated function signature now accepts an
rpcUrlparameter, which makes the utility more flexible by eliminating the dependency on a hardcoded RPC URL.
122-122: Good implementation of dynamic RPC URLThe client initialization now properly uses the provided
rpcUrlparameter instead of the hardcoded constant.
145-149: Enhanced error handling for GroupMessages contract deploymentThe added error checks using
require.NoErrorimprove the robustness of the test utility by preventing silent failures during contract deployment and initialization.
152-156: Enhanced error handling for IdentityUpdates contract deploymentSimilar to the GroupMessages contract, the added error checks ensure proper validation of contract deployment and initialization.
179-193: Updated public function signatures to support dynamic RPC URLsAll exported deployment functions now properly accept and pass along the
rpcUrlparameter, maintaining a consistent interface across the package.pkg/blockchain/ratesAdmin.go (1)
86-88: Added useful getter method for contract accessThis getter method provides controlled access to the internal contract instance, following good encapsulation practices while enabling test code to interact with the contract directly.
pkg/blockchain/rpcLogStreamer_test.go (3)
9-12: Added import for the new anvil packageThe import statement properly includes the new testing utility package needed for starting Anvil instances.
48-49: Improved test setup with Anvil integrationThe test now uses the new anvil utility to start a test Ethereum node and correctly registers the cleanup function using
t.Cleanup()to ensure resources are properly released after the test completes.
52-52: Properly using dynamic RPC URLThe client initialization now uses the RPC URL provided by the Anvil instance, ensuring consistency in the test environment.
pkg/blockchain/migrator/migrator_test.go (3)
13-13: Added import for the new anvil packageThe import statement properly includes the new testing utility package needed for starting Anvil instances.
22-24: Enhanced test setup with Anvil integrationThe
setupRegistryfunction now uses the anvil utility to start a test Ethereum node and creates contract options with the dynamic RPC URL. The contract deployment also correctly passes the RPC URL to maintain consistency.
51-51: Proper resource cleanupThe cleanup function correctly defers the anvil cleanup, ensuring that the Anvil instance is properly shut down when tests complete.
pkg/testutils/contracts_test.go (2)
8-8: Good addition of the anvil package import.The anvil package is properly imported to support the new test utility for starting Ethereum test nodes.
19-21: Consistent implementation across test functions.Good consistency in how Anvil is started and cleaned up across different test functions.
pkg/indexer/storer/identityUpdate_test.go (3)
18-18: Good addition of the anvil package import.The anvil package is properly imported to support starting Ethereum test nodes.
29-32: Good refactoring using the new Anvil utility.The code now properly starts an Anvil instance for testing and uses the new configuration approach with RPC URL. The contract deployment is handled appropriately.
45-45: Appropriate resource cleanup.Using
defer anvilCleanup()ensures the Anvil instance is properly cleaned up after the test completes.pkg/testutils/config.go (1)
16-23: Good refactoring of contract options.The new
NewContractsOptionsfunction improves flexibility by accepting a dynamic RPC URL parameter rather than using a hardcoded value, which aligns well with the Anvil utility's approach of using dynamically allocated ports.pkg/blockchain/blockchainPublisher_test.go (3)
9-11: Good organization of imports.The imports are well-organized with the new anvil package import.
19-27: Good implementation of Anvil for testing.The code properly starts an Anvil instance and deploys contracts with the right parameters. The comments explaining what's happening are helpful.
59-59: Good use of t.Cleanup instead of defer.Using
t.Cleanup(cleanup)is a better practice for tests thandefer cleanup()as it ensures cleanup happens even if the test fails. This change is consistent with Go testing best practices.Also applies to: 114-114
pkg/indexer/storer/groupMessage_test.go (3)
15-15: Good addition of the new anvil test utility package.This import enables the use of the new anvil testing utilities, which will help improve test reliability by providing a standardized way to start and manage Anvil blockchain instances.
25-28: Well-implemented Anvil instance initialization.The code effectively starts an Anvil instance for testing and properly configures the contracts using the RPC URL. This approach provides better isolation for tests compared to potentially shared test environments.
40-40: Proper resource cleanup with anvilCleanup.Good practice to include the Anvil cleanup function in the test teardown to ensure resources are properly released after test completion.
pkg/blockchain/ratesAdmin_test.go (6)
1-1: Good package naming convention applied.Changing to
blockchain_testfollows Go's recommended practice of using a separate package for tests. This avoids circular imports and provides clearer separation between implementation and tests.
11-13: Appropriate imports to support package refactoring.The addition of the blockchain package import is necessary due to the package name change, and the anvil package import enables the new testing approach.
16-16: Clear return type declaration.Explicitly specifying the return type as
*blockchain.RatesAdminimproves code readability and maintainability.
19-22: Well-structured Anvil setup with proper cleanup.Starting an Anvil instance and registering cleanup with
t.Cleanup()ensures reliable test execution and resource management. The switch toNewContractsOptionswith an explicit RPC URL makes the test setup more explicit.
53-54: Simplified error handling.Removed the
require.Eventuallywrapper, making the code more straightforward and easier to understand while maintaining the same functionality.
61-61: Direct contract method call improves readability.The direct call to
ratesAdmin.Contract().GetRateswithout therequire.Eventuallywrapper simplifies the code and maintains the same functionality.pkg/blockchain/registryAdmin_test.go (6)
1-1: Good package naming convention applied.Changing to
blockchain_testfollows Go's recommended practice for test packages, preventing circular imports and providing better separation.
9-11: Appropriate imports for package refactoring.Adding the blockchain package import is necessary after the package name change, and the anvil package import enables the new testing approach.
15-16: Improved type clarity in function signature.Using fully qualified types (
blockchain.INodeRegistryAdmin,blockchain.INodeRegistryCaller) improves code readability and maintainability.
19-24: Well-structured Anvil setup for testing.Starting an Anvil instance and using its RPC URL for contract deployment provides better test isolation and reliability.
41-41: Proper resource cleanup.Adding
defer cleanup()ensures the Anvil instance is properly terminated after the test completes, preventing resource leaks.
48-48: Improved type clarity in parameter.Using the fully qualified type
blockchain.INodeRegistryAdminimproves code readability and maintainability.pkg/indexer/e2e_test.go (6)
9-19: Good organization of imports.The addition of the required imports, especially the anvil package, supports the new testing approach.
24-26: Enhanced function signature with ContractsOptions.Returning
config.ContractsOptionsfrom the function allows for better reuse of configuration in other parts of the test.
31-35: Well-implemented Anvil setup.The code properly initializes an Anvil instance and deploys the necessary contracts, creating a self-contained test environment.
42-46: Comprehensive cleanup function.The cleanup function properly handles all resources, including the Anvil instance, database, and context cancellation.
49-54: Improved messagePublisher signature.Adding the
contractsCfgparameter allows the function to use the configuration fromstartIndexing, making the code more maintainable.
79-80: Updated function calls for new signatures.The code correctly captures and passes the contract configuration between functions.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
pkg/upgrade/docker_utils_test.go (1)
144-181: 🛠️ Refactor suggestionValidate container operation before termination
Currently, the container is immediately terminated once it signals a successful start. This confirms that the container can start, but it doesn’t validate runtime behavior or ensure the container is stable. To ensure meaningful testing, consider running additional checks or allowing the container to operate for a short time before termination.
🧹 Nitpick comments (25)
pkg/indexer/indexer.go (1)
304-338: Well-structured reorg handling logic with clear control flowThe reorg handling logic is now properly guarded by the
skipReorgHandlingflag, which provides clearer control flow compared to the previous implementation. The reorganization detection and processing remains intact while adding better resilience for blockchain connectivity issues.While this approach is sound, consider adding metrics or additional logging to track how frequently reorg handling is being skipped, as frequent skipping might indicate underlying issues with blockchain connectivity that should be addressed.
pkg/metrics/payer.go (2)
21-27: Minor naming inconsistency in metric name.There's a slight inconsistency in the naming pattern compared to other metrics.
- Name: "xmtp_payer_read_own_commit_in_time_seconds", + Name: "xmtp_payer__read_own_commit_in_time_seconds",This would make it consistent with the double underscore pattern used in the
xmtp_payer__node_publish_duration_secondsmetric.
34-44: Gauge metric with clear purpose.The gauge is well-designed and the comment about thread safety is helpful. Consider adding a brief inline comment explaining when this metric is updated in the application flow.
.github/workflows/upgrade-test.yml (1)
1-49: Workflow File is Fully Commented OutAll lines of this workflow are commented out, resulting in an empty workflow. If this is intentional (for example, if it serves as a template or is being reserved for future use), please add an explanatory comment. Otherwise, consider uncommenting the necessary lines so that the workflow is active and can be executed by GitHub Actions.
🧰 Tools
🪛 actionlint (1.7.4)
1-1: workflow is empty
(syntax-check)
go.mod (1)
32-56: Review of the Indirect Dependencies BlockSeveral indirect dependencies have been added or updated (e.g.,
dario.cat/mergo,github.com/AdaLogics/go-fuzz-headers, andgithub.com/cenkalti/backoff/v4among others). It is recommended to rungo mod tidyto ensure consistency and also double-check licensing and potential conflicts.pkg/upgrade/docker_utils_test.go (1)
122-142: Log pull progress to aid debuggingDiscarding the output from
dockerClient.ImagePullmakes it difficult to diagnose pull failures or track progress. Consider logging or buffering the pull output:- _, err = io.Copy(io.Discard, reader) + buf := new(bytes.Buffer) + _, err = io.Copy(buf, reader) + if err == nil { + fmt.Printf("Pull logs for %s:\n%s\n", imageName, buf.String()) + }pkg/upgrade/main_test.go (1)
26-33: Consider pulling images in parallelPulling images sequentially can significantly prolong setup time, especially if the images are large or if there are many. You could optimize this by spawning goroutines for each image pull, then using a sync.WaitGroup to coordinate. However, be mindful of Docker rate limits and network capacity.
pkg/upgrade/upgrade_test.go (2)
11-20: Manage version list maintenanceHardcoding a list of versions is fine for controlled tests, but can get outdated. For a dynamic or regularly updated environment, consider an external registry query or environment variable. This prevents stale versions in longer-lived test suites.
52-63: Strengthen test coverage for the “latest version”You only verify that the dev container starts successfully. Adding more container validations—like checking logs for expected service readiness or running a smoke test against an endpoint—would provide stronger confidence in the container’s correctness.
contracts/src/Nodes.sol (3)
13-14: Consider removing or addressing the TODOs.
These TODO comments are minimal but call attention to potentially unnecessary duplication of events and incomplete error handling. Clarifying or removing them ensures the code remains tidy and free of unimplemented references.
103-103: Validate overflow boundary for_nodeCounter.
Whileuint32provides a large range, it may eventually overflow if many nodes are minted over the contract’s lifetime. Consider either enforcing a cap or documenting the maximum possible number of nodes in order to avert future issues.
328-365: Consolidate API and replication activation logic if possible.
The_activateApiNodeand_activateReplicationNode(and their disable counterparts) are nearly symmetrical. A shared internal utility could reduce code duplication, although it’s optional.contracts/src/IdentityUpdates.sol (2)
22-22: Consider indexing event parameters.
The TODO note mentions indexinginboxIdandsequenceId. Doing so can facilitate quicker and more efficient off-chain filtering and lookups of these events.
128-135: Large maximum payload size may lead to high gas usage.
The upper bound of over 4 MB (4,194,304 bytes) could excessively increase gas consumption and possibly lead to out-of-gas errors. Consider whether storing such large payloads on-chain is necessary.contracts/src/RatesManager.sol (4)
9-10: Consider making PAGE_SIZE configurable or clarifying the approach.
The TODO suggests thatPAGE_SIZEshould be a default value that can be overridden by the caller, but there's no mechanism for doing that yet. Either implement a function to override it or clarify that it's fixed in this contract if that's deliberate.
18-23: Index bothupgraderandnewImplementationevents.
The comment indicates a wish to index the event parameters. For more efficient filtering and event queries, consider marking one or both fields asindexed.-event UpgradeAuthorized(address upgrader, address newImplementation); +event UpgradeAuthorized(address indexed upgrader, address indexed newImplementation);
72-72: Use custom error for clarity and consistency.
Currently, the code callsrequire(admin != address(0), ZeroAdminAddress());. Custom errors are typically used withrevert. Consider:- require(admin != address(0), ZeroAdminAddress()); + if (admin == address(0)) { + revert ZeroAdminAddress(); + }This ensures you get the benefits of custom error encoding.
168-173: Add a code-size or contract-checking mechanism.
_authorizeUpgradechecks for a zero address but does not confirm there is actual code atnewImplementation. Consider verifying the code size to mitigate potential malicious or empty upgrades.contracts/test/IdentityUpdates.t.sol (1)
44-45: Improve revert message for zero admin address.
You’re reverting withIdentityUpdates.ZeroAdminAddress.selector. To make debugging easier, consider including a short revert reason or an event that logs the invalid address.contracts/test/RatesManager.t.sol (2)
65-76: Check for admin vs. manager roles.
The test includes a TODO comment about verifying when the admin is not the manager. Consider adding additional tests for a scenario in which these roles are separate addresses to confirm the role-based restrictions.
248-258: Ensure new implementation code presence.
Similar to the earlier file, you revert withRatesManager.ZeroImplementationAddress.selectorbut do not verify that the pointed address is a valid contract. You might want to checkextcodesize(newImplementation)to reduce risk of upgrading to an uninitialized or non-contract address.contracts/src/GroupMessages.sol (4)
9-10: Clarify TODO items.
The TODO comments “IGroupMessages” and “Abstract PayloadBroadcaster” remain unaddressed. Consider removing these if they’re no longer necessary or opening a follow-up task to finalize these abstractions.
16-22: Indexing recommendation for event parameters.
ThegroupIdandsequenceIdparameters are likely good candidates to index in this event, as the TODO suggests. Indexing these can improve on-chain filtering capabilities and ease of analytics.event MessageSent( - bytes32 groupId, - bytes message, - uint64 sequenceId + bytes32 indexed groupId, + bytes indexed message, + uint64 indexed sequenceId );
24-30: Consider indexing fields in upgrade event.
To align with best practices, indexingupgraderandnewImplementationmight further facilitate event tracking, consistent with the TODO.
189-197: Upgrade authorization logic.
- Checking
newImplementation != address(0)is crucial.- Emitting
UpgradeAuthorizedevent for transparency is good.
Consider finishing the TODO to check if code is present atnewImplementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
.github/workflows/test.yml(0 hunks).github/workflows/upgrade-test.yml(1 hunks)contracts/foundry.toml(1 hunks)contracts/script/DeployGroupMessages.s.sol(1 hunks)contracts/script/DeployIdentityUpdates.s.sol(1 hunks)contracts/script/DeployNodeRegistry.s.sol(1 hunks)contracts/script/DeployRatesManager.s.sol(1 hunks)contracts/script/upgrades/UpgradeGroupMessages.s.sol(1 hunks)contracts/script/upgrades/UpgradeIdentityUpdates.s.sol(1 hunks)contracts/script/utils/Environment.sol(1 hunks)contracts/script/utils/Utils.sol(2 hunks)contracts/src/GroupMessages.sol(1 hunks)contracts/src/IdentityUpdates.sol(1 hunks)contracts/src/Nodes.sol(9 hunks)contracts/src/RatesManager.sol(2 hunks)contracts/src/interfaces/INodes.sol(4 hunks)contracts/src/interfaces/IPayer.sol(0 hunks)contracts/src/interfaces/IPayerReport.sol(5 hunks)contracts/test/GroupMessage.t.sol(3 hunks)contracts/test/IdentityUpdates.t.sol(3 hunks)contracts/test/Nodes.sol(0 hunks)contracts/test/Nodes.t.sol(1 hunks)contracts/test/RatesManager.t.sol(1 hunks)contracts/test/utils/Harnesses.sol(1 hunks)contracts/test/utils/Utils.sol(1 hunks)go.mod(7 hunks)pkg/api/payer/nodeCursorTracker.go(3 hunks)pkg/api/payer/service.go(5 hunks)pkg/blockchain/blockchainPublisher_test.go(3 hunks)pkg/blockchain/nonceManager.go(2 hunks)pkg/indexer/indexer.go(1 hunks)pkg/metrics/metrics.go(1 hunks)pkg/metrics/payer.go(1 hunks)pkg/upgrade/docker_utils_test.go(2 hunks)pkg/upgrade/main_test.go(1 hunks)pkg/upgrade/upgrade_test.go(1 hunks)
💤 Files with no reviewable changes (3)
- contracts/test/Nodes.sol
- contracts/src/interfaces/IPayer.sol
- .github/workflows/test.yml
✅ Files skipped from review due to trivial changes (2)
- contracts/src/interfaces/IPayerReport.sol
- contracts/src/interfaces/INodes.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/blockchain/blockchainPublisher_test.go
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/upgrade-test.yml
1-1: workflow is empty
(syntax-check)
🔇 Additional comments (142)
contracts/script/utils/Utils.sol (3)
4-4: Import statement refactored for better clarity.The import statement has been updated to use destructured imports, which explicitly states what is being imported from the module. This is a good practice as it makes dependencies clearer and helps prevent naming conflicts.
7-9: Improved numeric literal readability with underscores.The chain ID constants have been reformatted with underscores for better readability, which is a good practice in Solidity for large numeric values. This change maintains the same values while making the code more maintainable.
45-55: Function refactored to use early returns pattern.The
_resolveChainIDfunction has been refactored to remove nested if-else blocks in favor of early returns. This simplifies the control flow and makes the code more readable without changing its functionality.The new structure is cleaner and follows the principle of reducing cognitive load by eliminating nested conditionals.
pkg/indexer/indexer.go (1)
289-302: Improved error handling for blockchain query failuresThe introduction of the
skipReorgHandlingflag with a warning instead of completely skipping the event (viacontinue) is a good improvement. This change allows event processing to continue even when blockchain query operations fail, making the indexer more resilient against temporary blockchain connectivity issues.contracts/foundry.toml (3)
22-24: New Fuzz Section Configuration AddedThe new
[fuzz]section is introduced withruns = 10, which appears suitable for basic local fuzz testing. Please verify that this default value meets your intended testing sensitivity and can be adjusted if future requirements dictate more iterations.
25-27: CI Fuzz Profile ConfigurationThe
[profile.ci.fuzz]section setsruns = 10_000, promoting more exhaustive fuzz testing in a CI environment. Ensure that your CI pipeline has sufficient resources and that this run count does not adversely extend test durations.
28-41: Formatting Configuration AddedThe addition of the
[fmt]section with detailed formatting options (such as preserving single-line statement blocks, specific line lengths, and tab widths) is a good step toward enforcing consistent style. Please confirm that these settings align with your team's style guidelines and are supported by the current version of Foundry.contracts/test/utils/Utils.sol (4)
5-6: Approve renaming of EIP1967 constant for improved clarity.The constant has been renamed from
EIP1967_IMPL_SLOTtoEIP1967_IMPLEMENTATION_SLOT, which enhances readability by using the full term "IMPLEMENTATION" instead of the abbreviated "IMPL". This follows best practices for naming constants in smart contracts.
9-13: Good improvement to function signature and loop initialization.Two improvements in this segment:
- The return variable
payloadis now declared directly in the function signature, which is a good Solidity practice that improves readability and documentation.- The loop variable
iis initialized without an explicit value (which defaults to zero in Solidity), following modern Solidity conventions.
19-22: Approve formatting improvement for better readability.The return statement has been reformatted across multiple lines, which improves readability without changing the functionality. This makes the complex expression easier to understand and maintain.
24-27: Consistent improvement to function signature and initialization.Similar to the changes in
_generatePayload:
- The return variable
messageis now declared in the function signature.- Loop variable initialization follows the same pattern of implicit zero initialization.
These changes maintain consistency throughout the codebase and follow modern Solidity best practices.
pkg/metrics/metrics.go (1)
75-79: LGTM! New metrics additions for observability.Five new metrics collectors have been added to track various aspects of the system:
- nodePublishDuration
- cursorBlockTime
- currentNonce
- payerBanlistRetry
- messagesOriginated
These additions enhance system observability without changing behavior.
pkg/blockchain/nonceManager.go (2)
7-7: LGTM! New import for metrics package.Clean import added for metrics collection.
88-88: LGTM! Enhanced observability for nonce tracking.Good addition that emits metrics for current nonce values, enhancing monitoring capabilities without altering behavior.
pkg/api/payer/nodeCursorTracker.go (3)
6-7: LGTM! Required imports for new metrics tracking.Added imports for metrics package and time needed for performance measurement.
Also applies to: 11-11
50-51: LGTM! Performance tracking initialization.Good addition that initializes a timestamp for measuring the duration of cursor tracking.
105-108: LGTM! Enhancing observability with metrics emission.Valuable addition that records and emits metrics related to cursor tracking duration, providing insights into system performance.
pkg/api/payer/service.go (4)
6-6: LGTM! Added metrics import.Clean import added for metrics collection.
184-186: LGTM! Conditional metrics emission for retries.Good addition that only emits metrics when retries actually occur (retries != 0), avoiding unnecessary metric entries.
225-225: LGTM! Performance tracking for node publishing.Good additions that:
- Track the time taken for node publishing operations
- Record the number of messages originated
These metrics enhance observability of the service's performance.
Also applies to: 233-234
268-269: LGTM! Performance tracking for blockchain publishing.Well-implemented metrics tracking for blockchain publishing operations, measuring both duration and message count, similar to the node publishing metrics.
Also applies to: 329-331
pkg/metrics/payer.go (5)
1-6: Clean and appropriate imports.The imports for Prometheus and string conversion are appropriate for the metrics functionality being implemented.
8-19: Well-structured node publish duration metric.The histogram is correctly set up with appropriate naming following Prometheus conventions, and the helper function properly handles type conversion for labels.
46-58: Well-structured histogram with custom buckets.The custom buckets [0-5] are appropriate for counting retry attempts. The metric name is descriptive, though quite long.
60-71: Counter implementation follows best practices.The counter for tracking originated messages is correctly implemented with proper labeling and a clear helper function.
1-71:Details
✅ Verification successful
Ensure metrics are registered correctly.
While the metrics are well-defined, ensure they're properly registered with Prometheus in the
registerCollectorsfunction (likely inmetrics.go).
🏁 Script executed:
#!/bin/bash # Check if these metrics are registered in the metrics.go file grep -n "registerCollectors" pkg/metrics/metrics.go && \ grep -A 15 "registerCollectors" pkg/metrics/metrics.go | grep -E "nodePublishDuration|cursorBlockTime|currentNonce|payerBanlistRetry|messagesOriginated"Length of output: 389
Prometheus Metrics Registration Verified
The metrics defined in
pkg/metrics/payer.go(i.e.,nodePublishDuration,cursorBlockTime,currentNonce,payerBanlistRetry, andmessagesOriginated) are properly registered in theregisterCollectorsfunction withinpkg/metrics/metrics.go.
- Confirmed that the registration code includes all the metrics in question.
- No additional changes are required.
go.mod (4)
5-21: Updated Dependency Versions in the First require BlockNotice the version updates for dependencies such as:
github.com/grpc-ecosystem/grpc-gateway/v2updated to v2.26.1,google.golang.org/genproto/googleapis/apiupdated tov0.0.0-20250218202821-56aae31c358a, andgoogle.golang.org/protobufupdated to v1.36.5.Please ensure that these version bumps are compatible with existing code and that no unexpected breaking changes are introduced.
23-30: Addition of New Dependencies in the Second require BlockNew dependencies such as
github.com/docker/docker v27.5.0+incompatibleandgithub.com/testcontainers/testcontainers-go v0.35.0have been added. These appear aligned with the objectives for enhanced testing and container management. Verify that these dependencies meet your requirements and are correctly integrated.
163-167: Updated OpenTelemetry DependenciesThe OpenTelemetry-related dependencies have been updated to v1.35.0:
go.opentelemetry.io/otelgo.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttpgo.opentelemetry.io/otel/metricgo.opentelemetry.io/otel/sdkgo.opentelemetry.io/otel/tracePlease confirm that these updates integrate smoothly with your observability instrumentation and do not introduce breaking changes.
188-189: Replacement Directive for go-ethereumThe
replacedirective on line 188 substitutesgithub.com/ethereum/go-ethereumwithgithub.com/OffchainLabs/go-ethereumat a specific commit/version. Ensure that this replacement is fully compatible with the components relying ongo-ethereum, and that the new version satisfies both current and projected requirements.pkg/upgrade/docker_utils_test.go (1)
9-24: Check for unused imports if anyThese newly added imports (Docker client, testcontainers, etc.) align with your container management approach. Ensure that each import is truly needed and remove any that remain unused to keep the import list clean.
pkg/upgrade/upgrade_test.go (1)
4-8: New imports are appropriateImporting
contextandrequirefor test usage is a standard practice. They look correct here for managing test container startup and asserting conditions, respectively.contracts/src/Nodes.sol (2)
128-135: Approve removal from API and replication sets on node removal.
The approach of calling_disableApiNodeand_disableReplicationNodeensures consistency. This cleanly removes the node from the corresponding sets without duplication in logic.
207-210: Good use of internal checks for node ownership and status.
The_revertIfNodeDoesNotExist,_revertIfNodeIsDisabled, and_revertIfCallerIsNotOwnerchecks provide well-structured validations. They increase maintainability and readability.contracts/src/IdentityUpdates.sol (2)
84-98: Initialization pattern looks correct.
Usinginitializerwithrequire(admin != address(0))helps prevent misconfiguration. GrantingDEFAULT_ADMIN_ROLEto the specified admin is consistent with best practices.
192-196: Sufficient protection during contract upgrade.
Requiring a non-zeronewImplementationaddress and having a dedicated event mitigate accidental zero upgrades and provide clarity on upgrade events.contracts/script/utils/Environment.sol (1)
4-4:Details
✅ Verification successful
Ensure removal of
Scriptinheritance does not affect dependents.
Since you’ve removedis Script, verify that no essential functionality (e.g., console logging, script-based tools) is lost. If referencingScriptfunctionalities elsewhere, consider an alternative approach or library.
🏁 Script executed:
#!/bin/bash # Confirm no references remain to the `Script` contract or related functionality. rg 'Script' --context 3Length of output: 2452
Script Inheritance Removal Verified
The removal of
is Scriptfromcontracts/script/utils/Environment.solappears intentional and isolated. Our search confirms that:
- Environment.sol no longer inherits from
Scriptas expected.- Other parts of the repository (e.g., in
contracts/script/utils/Utils.sol) still reference and inherit fromScript, which seems to be intentional for those utilities.Please verify that any functionality previously provided by
Scriptin the Environment context is either non-essential or has been adequately replaced. Otherwise, the current changes look safe.contracts/script/DeployNodeRegistry.s.sol (1)
4-9:Details
❓ Verification inconclusive
Refactored import statements and contract inheritance
The changes remove the dependency on Forge's
Scriptbase contract while maintaining functionality through theUtilsandEnvironmentimports. This refactoring appears to be part of a broader change across deployment scripts to standardize how they're structured.Make sure the
vmfunctions used throughout the contract (likevm.envAddress,vm.startBroadcast, etc.) are properly accessible through the remaining imports (UtilsorEnvironment).
🏁 Script executed:
#!/bin/bash # Check if vm functions are available in Environment.sol or Utils.sol echo "Checking for vm accessibility in Utils.sol and Environment.sol" grep -r "vm\." ./contracts/script/utils/Length of output: 627
Review: Verify Availability of All Required
vmFunctionsThe refactoring to remove the Forge
Scriptbase while relying onUtilsandEnvironmentappears sound. Our grep check confirms thatUtils.solprovides access to severalvmfunctions (e.g.,vm.readFile,vm.writeJson, andvm.projectRoot). However, please double-check that all othervmfunctions referenced in the deployment scripts—specifically functions such asvm.envAddressandvm.startBroadcast—are available through eitherUtilsorEnvironment. If these functions are inherited from Forge’s cheatcode interface or are wrapped in one of the imported contracts, then no further changes are required.
- File: contracts/script/DeployNodeRegistry.s.sol (lines 4-9)
- Note: Confirm that any deployment calls relying on
vm.envAddressorvm.startBroadcastfunction correctly with the new refactored imports.contracts/script/upgrades/UpgradeGroupMessages.s.sol (1)
4-12:Details
❓ Verification inconclusive
Refactored imports and contract inheritance structure
The contract's import statements have been reorganized and the inheritance has been simplified by removing
Script. The addition ofstdJsonand explicit import ofGroupMessageswith proper path makes dependencies clearer.Ensure that
vmfunctionality (used in multiple places likevm.envUint,vm.startBroadcast, etc.) is still accessible through the remaining imports (UtilsorEnvironment).
🏁 Script executed:
#!/bin/bash # Verify if the import paths for GroupMessages are correct cat ./contracts/src/GroupMessages.sol 2>/dev/null || echo "GroupMessages.sol not found at expected path" # Check if stdJson is properly accessible grep -r "stdJson" ./contracts/script/upgrades/UpgradeGroupMessages.s.solLength of output: 7818
Reorganized Imports and Inheritance Structure Verified
- The explicit import of
GroupMessagesfrom../../src/GroupMessages.solis correct and the file exists as expected.- The import and usage of
stdJsonfrom"forge-std/src/StdJson.sol"are confirmed.- Action Required: Please double-check that
vmfunctionality (e.g.,vm.envUint,vm.startBroadcast, etc.) remains accessible through theUtilsorEnvironmentcontracts after removing the inheritance fromScript.contracts/script/DeployRatesManager.s.sol (1)
4-11: Refactored imports and contract inheritanceThe import structure has been reorganized to be more explicit, and the dependency on Forge's
Scriptcontract has been removed. The contract now directly inherits only fromUtilsandEnvironment.This change aligns with the pattern seen in other deployment scripts, creating a more consistent approach across the codebase.
contracts/script/DeployGroupMessages.s.sol (1)
4-11: Refactored imports and contract inheritance structureThe contract's import statements have been reorganized and the inheritance has been simplified by removing
Script. The explicit import ofGroupMessageswith proper namespace improves code clarity.This change is consistent with the refactoring pattern seen in the other deployment scripts, creating a more uniform approach across the codebase.
contracts/script/upgrades/UpgradeIdentityUpdates.s.sol (7)
4-5: Imports forstdJsonandERC1967Proxyare consistent with usage.
These imports appear necessary for JSON parsing and proxy upgrades, and there are no immediate concerns.
6-6: No impact change.
This blank line likely represents a minor formatting or spacing adjustment.
7-7: Confirm correct relative path forIdentityUpdates.
Ensure references to../../src/IdentityUpdates.solmatch the desired file structure.
8-8: No impact change.
This blank line likely represents a minor formatting or spacing adjustment.
9-10: ImportsUtilsandEnvironmentappear valid.
These utilities likely provide shared functionalities. Confirm thatUtilsandEnvironmentproperly expose thevmcontext as needed.
11-11: No impact change.
This blank line likely represents a minor formatting or spacing adjustment.
12-12: Verify removal ofScriptfrom inheritance.
Confirm that calls tovm.*and otherScriptfunctionalities do not break with the removal ofScriptas a base class.contracts/script/DeployIdentityUpdates.s.sol (7)
4-4: OpenZeppelin proxy import is appropriate.
This matches the usage forERC1967Proxydeployment.
5-5: No impact change.
This blank line likely represents a minor formatting or spacing adjustment.
6-6: Check new import path forIdentityUpdates.
Confirm usage across the codebase matches this updated relative path.
7-7: No impact change.
This blank line likely represents a minor formatting or spacing adjustment.
8-9: Reorganized imports forUtilsandEnvironment.
No issues noted. These imports likely provide essential test or environment functionality.
10-10: No impact change.
This blank line likely represents a minor formatting or spacing adjustment.
11-11: Confirm removal ofScriptfrom inheritance.
Ensurevmusage and anyScript-provided utilities are still accessible throughUtilsor other inherited contracts.contracts/test/utils/Harnesses.sol (7)
1-3: File licensing and pragma are fine.
UsingSPDX-License-Identifier: MITandpragma solidity 0.8.28;is standard.
4-5: ImportingEnumerableSetfrom OpenZeppelin.
This is a standard library import, no concerns.
6-10: Base contract imports check.
Imports forGroupMessages,IdentityUpdates,Nodes, andRatesManagerare aligned with harness usage.
11-35:GroupMessagesHarness
Exposes internal storage and pause/unpause methods for testing. This is a typical harness pattern. No immediate problems detected.
37-61:IdentityUpdatesHarness
Similar harness approach, exposing internal state for testing pause/unpause, sequence ID, and payload size. Implementation appears consistent.
63-132:NodesHarness
Provides a comprehensive set of external methods to manipulate node state in tests. This is standard for advanced test scenarios. Ensure usage is restricted to testing only.
134-152:RatesManagerHarness
Exposes internal state push & retrieval methods for rates. No immediate correctness issues.contracts/test/Nodes.t.sol (28)
1-3: File licensing and pragma are fine.
Adheres to MIT license, withpragma solidity 0.8.28;.
4-5: ImportingTestfrom forge-std.
Ensures Foundry’s testing utilities are available. No issues noted.
20-24: Constants and role definitions.
DefinesDEFAULT_ADMIN_ROLE,ADMIN_ROLE, andNODE_MANAGER_ROLEfor clarity. Looks correct.
38-43:setUpfunction.
InitializesNodesHarnesswith anadminand grantsNODE_MANAGER_ROLEtomanager. Straightforward approach.
53-79:test_addNode_first
Tests initial node addition. The usage ofvm.expectEmitand subsequent assertions are thorough.
138-156:test_addNode_notAdmin
Ensures revert behavior for unauthorized calls. Proper usage of role checks andvm.expectRevert.
188-211:test_disableNode
Validates disabling a node, ensuring correct state changes and event emissions. Comprehensive coverage.
293-316:test_transferFrom
Tests the transfer flow, verifying that active node sets are cleared and events are emitted. Good coverage for ownership transfer logic.
406-460: API enable/disable logic.
Confirms correct revert scenarios if the node is disabled or the caller is unauthorized. Thorough checks.
464-516: Replication enable/disable logic testing.
Similar approach to API enabling with robust revert checks. Implementation is consistent.
520-557:test_setMinMonthlyFee
Validates manager-only assignment for monthly fees. Proper usage ofvm.expectRevertfor unauthorized roles.
560-569:test_setMaxActiveNodes
Checks admin-right flows and coverage for updating the maximum active nodes. Implementation looks correct.
598-608: Node operator commission percentage updates.
Ensures the commission remains within valid bounds (MAX_BPS).
628-636:test_setBaseURI
Covers event emission and internal base URI updates. Straightforward approach.
670-689:test_getAllNodes
Checks node retrieval after multiple additions, verifying correctness of returned arrays.
725-744:test_getActiveApiNodes
Ensures that active node sets are tracked accurately.
748-767:test_getActiveReplicationNodes
Similar test confirming replication nodes are tracked correctly.
771-782:test_getActiveApiNodesIDs
Demonstrates retrieval of IDs. Implementation is consistent.
786-797:test_getActiveReplicationNodesIDs
Mirrors the API node approach for replication nodes.
801-807:test_getActiveApiNodesCount
Validates the count of active API nodes. Straightforward logic.
811-817:test_getActiveReplicationNodesCount
Checks the count for active replication nodes. No concerns.
821-830:test_getApiNodeIsActive
Ensures correct boolean checks for active nodes.
834-843:test_getReplicationNodeIsActive
Analogous test for replication nodes. Implementation is parallel.
847-852:test_supportsInterface
Verifies compliance with ERC721, IERC165, and AccessControl interfaces. Good coverage.
856-859:test_revokeRole_revokeDefaultAdminRole
Properly checks for revert on default admin role revocation. Implementation aligns with contract constraints.
863-869:test_renounceRole_withinDelay
Confirms that role renunciation within an enforced admin delay is disallowed.
873-894: Helper_addNode
Facilitates manual node creation for testing. Exposes harness methods and mints the token.
895-904: Helper_getRandomNode
Generates pseudo-random node data. Standard approach for test variability.contracts/src/RatesManager.sol (1)
139-139: Check for empty list logic.
Thisrequire(fromIndex < $.allRates.length, FromIndexOutOfRange());is correct for non-empty arrays, but the function’s behavior for an empty array is handled by a separate condition at lines 137–138. Ensure both checks stay consistent and do not introduce edge-case off-by-one issues.contracts/test/IdentityUpdates.t.sol (4)
17-18: Good use of utility inheritance.
Extending bothTestandUtilscentralizes repeated functionality and improves maintainability of cross-cutting utilities or test helpers.
125-161: Comprehensive fuzz testing.
Your fuzz testing foraddIdentityUpdatecovers a broad range of payload sizes and states (paused/unpaused). This provides good real-world coverage and reduces edge-case bugs.
249-252: Prevent re-initialization.
This test correctly checks thatinitializereverts if called on a contract that’s already initialized, enforcing the OpenZeppelin initialization pattern. Well done.
311-316: Clear revert condition for unexpected unpause behavior.
test_unpause_whenNotPausedensures thatunpause()reverts if the contract is already unpaused. This is an excellent defensive check.contracts/test/RatesManager.t.sol (3)
9-10: Imports for upgradeable contracts.
These import statements improve clarity around the contract's upgradeable nature. Make sure you’re following recommended versions to align with upstream security patches for proxies.
17-20: Consistent usage of roles and structured constants.
DefiningDEFAULT_ADMIN_ROLEandRATES_MANAGER_ROLEat the top clarifies their usage and promotes consistency across tests.
126-133: Verify the emptygetRatescall logic.
When the array is empty, callinggetRates(0)returns an empty array andfalseforhasMore. If you expect multiple usage patterns, be sure to confirm that no out-of-bounds condition arises for any future extension.contracts/src/GroupMessages.sol (14)
4-7: Good use of upgradeable imports.
These imports from@openzeppelin-contracts-upgradeableare appropriate for implementing UUPS capabilities, role-based access control, and pausable functionality.
31-35: Event documentation is clear.
The comment block adds clarity about the minimum payload size update. No issues found.
38-42: Event documentation for max payload size is consistent.
Nicely parallels the min payload size event.
45-51: New custom errors improve clarity.
The introduction ofZeroImplementationAddress()helps differentiate specific upgrade-related errors. This is good for debugging.
53-56: Constants for payload size.
DefiningABSOLUTE_MIN_PAYLOAD_SIZEandABSOLUTE_MAX_PAYLOAD_SIZEprovides clarity and helps prevent accidental misuse. Nicely done.
58-60: UUPS Storage documentation.
The storage comment clarifies persisting data across upgrades. This ensures devs understand the custom storage location.
61-70: Struct-based storage approach.
Using a dedicated struct for the contract’s storage (via_getGroupMessagesStorage) encapsulates the data and simplifies future upgrades. This is a recommended pattern for UUPS.
71-76: Assembly usage in_getGroupMessagesStorage().
The inline assembly to map a constant storage slot is valid. Just ensure that modifications to this slot remain consistent with upgrade-safe patterns.
80-98: Initialization logic.
The require statement to checkadmin != address(0)and custom error usage are correct. The storage initialization with min/max is consistent with the newly introduced struct. Nicely handled.
102-116: Pause and unpause functions.
Restricting these calls toonlyRole(DEFAULT_ADMIN_ROLE)and integrating the_pause/_unpausepattern fromPausableUpgradeableadheres to best practices. The approach looks solid.
120-137:addMessagefunction.
- Validates payload size rigorously with
InvalidPayloadSizeerror.- Uses
uncheckedfor incrementing sequence ID to save gas, which is acceptable since overflow is unlikely foruint64.
Overall, a solid and well-structured approach.
141-156: Setter for min payload size.
- Valid check ensuring
minPayloadSizeRequest <= maxPayloadSizeand bounding it aboveABSOLUTE_MIN_PAYLOAD_SIZE.- Emitting
MinPayloadSizeUpdatedkeeps the external usage in sync.
Implementation is well-formed.
158-173: Setter for max payload size.
- Properly checks
maxPayloadSizeRequest >= minPayloadSizeand<= ABSOLUTE_MAX_PAYLOAD_SIZE.- Emits
MaxPayloadSizeUpdated.
No issues observed.
177-185: Getter functions usage.
ExposingminPayloadSize()andmaxPayloadSize()is straightforward and aligns with the storage struct.contracts/test/GroupMessage.t.sol (30)
4-5: Consistent usage of forge-std & IAccessControl.
ImportingTestfor Foundry-based testing and usingIAccessControlfor role checks is aligned with the rest of the code.
8-11: Proxy and upgradeable dependencies.
These imports from ERC1967 and OpenZeppelin upgradeable contracts are consistent with the usage in the main contract.
12-15: Test harness usage.
By importingGroupMessagesHarnessfor more direct testing, you can assert internal states. Also,Utilscan help unify repeated logic. This approach is robust.
17-22: Defining test contract constants.
Clearly namingABSOLUTE_MIN_PAYLOAD_SIZEandABSOLUTE_MAX_PAYLOAD_SIZEin tests ensures alignment with the contract’s definitions.
23-28: Storing references to implementation and harness.
KeepinggroupMessagesImplementationandgroupMessagesin separate variables helps with clarity when upgrading or re-deploying the proxy.
31-40:setUp()function with proxy deployment.
The usage ofERC1967Proxypaired withGroupMessages.initialize.selectoris correct. This flow ensures the underlying implementation is properly set with the correct admin.
44-50: Zero admin address revert test.
Ensures that callinginitializewith address(0) is disallowed. This test is effectively verifying the custom error in the contract. Great coverage.
54-59:test_initialState()thoroughness.
- Checking implementation slot after proxy creation
- Verifying min/max payload size
- Confirming initial
sequenceIdis 0All are fundamental checks ensuring the contract was set up correctly.
63-72:test_addMessage_minPayload()
- Utilizes
_generatePayloadand expectsMessageSentevent.- Verifies
sequenceId.
Aligned with the contract logic.
74-83:test_addMessage_maxPayload()
Similarly tests the upper bound, ensuring behavior parallels min payload tests.
85-98: Tests for payload too small.
Confirms contract reverts withInvalidPayloadSize. Important negative test coverage.
100-113: Tests for payload too large.
Again, checks the revert scenario and ensures coverage of the upper bound logic.
115-123: Check revert when paused.
Ensures that callingaddMessagetriggersPausableUpgradeablerevert. This maintains consistency with the main contract’s Pause/Unpause approach.
125-161:testFuzz_addMessage
- Using Foundry’s fuzzing approach to bound the parameters covers a wide range of input sizes.
- The logic toggling
pausedtests both normal and revert scenarios.- Checking final
sequenceIdensures correctness upon success.Comprehensive test design.
165-174:test_setMinPayloadSize_notAdmin& invalid requests.
Covers role-based restriction (reverts for unauthorized) and boundary checks. This is thorough.
185-190: Rejected requests below absolute min.
Reusing theInvalidMinPayloadSizeerror ensures the contract’s constraints are enforced.
192-203:test_setMinPayloadSize()
- Emits updated event.
- Asserts new min size via accessor.
This completes a positive path test.
207-216:test_setMaxPayloadSize_notAdmin& boundary check.
Similar approach ensures that only privileged addresses can make changes and that invalid sizes revert correctly.
218-225: Reject requests below min.
Guards the contract from an invalid range. This aligns withInvalidMaxPayloadSizeusage.
227-232: Reject requests above absolute max.
Again, consistent usage ofInvalidMaxPayloadSize.
234-245:test_setMaxPayloadSize()
- Valid scenario test.
- Ensures the event is emitted, followed by correct assignment.
No issues found.
249-252: Prevent re-initialization.
Ensuresinitializecan only be called once, verifying theInvalidInitializationrevert. Good protection.
256-264:test_pause()
- Checks that only
admincan pause.- Asserts the state of
paused().
Proper usage ofPausableUpgradeable.
266-276:test_pause_notAdmin()&test_pause_whenPaused()
- Confirms revert if unauthorized calls
pause.- Confirms revert if already paused.
These negative paths are crucial for robust coverage.
288-298:test_unpause()
- Pauses first, then unpauses.
- Verifies the correct event is emitted.
Clear coverage for toggling the paused state.
300-309:test_unpause_notAdmin()&test_unpause_whenNotPaused()
Verifies revert conditions for unauthorized or redundant unpause calls. Thorough approach.
320-330:test_upgradeToAndCall_notAdmin()
Correctly ensures only the admin can upgrade.
332-337: Zero implementation address revert.
Prevents accidental bricking of the proxy, aligning with the contract’s custom error.
339-357: Successful upgrade preserving state.
- Verifies old state after upgrade (max/min payload size and
sequenceId).- Emits
UpgradeAuthorizedevent.
This final test ensures the upgrade path works as desired.
361-364: Helper function_getImplementationFromSlot().
Directly checking the proxy’s storage is an effective approach to confirm the correct implementation.
| ctx := context.Background() | ||
| for _, version := range xmtpdVersions { | ||
| image := fmt.Sprintf("%s:%s", ghcrRepository, version) | ||
|
|
||
| t.Run(version, func(t *testing.T) { | ||
| envVars := constructVariables(t) | ||
| t.Logf("Starting old container") | ||
| runContainer(t, fmt.Sprintf("xmtpd_test_%s", version), image, envVars) | ||
| err := runContainer( | ||
| t, | ||
| ctx, | ||
| image, | ||
| fmt.Sprintf("xmtpd_test_%s", version), | ||
| envVars, | ||
| ) | ||
| require.NoError(t, err, "Failed to start container version %s", version) | ||
|
|
||
| t.Logf("Starting new container") | ||
| runContainer(t, "xmtpd_test_dev", "ghcr.io/xmtp/xmtpd:dev", envVars) | ||
| err = runContainer( | ||
| t, | ||
| ctx, | ||
| "ghcr.io/xmtp/xmtpd:dev", | ||
| "xmtpd_test_dev", | ||
| envVars, | ||
| ) | ||
| require.NoError(t, err, "Failed to start dev container") | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify or refine “upgrade” testing logic
The test starts and immediately terminates old containers before spinning up the new container, so it's more like separate startup checks than a true upgrade path. If a rolling or in-place upgrade is desired, consider adapting the test to preserve old container data or run them concurrently to validate upgrade steps.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contracts/pkg/identityupdates/IdentityUpdates.go (1)
205-265: Consider documenting the relationship between absolute and configurable size limits.While the implementation is correct, it might be helpful to add documentation comments explaining the relationship between the absolute constants (
ABSOLUTEMAXPAYLOADSIZE/ABSOLUTEMINPAYLOADSIZE) and the configurable values (MaxPayloadSize/MinPayloadSize). This would clarify that the absolute values represent hard limits that cannot be exceeded, while the configurable values can be changed within those boundaries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
contracts/pkg/groupmessages/GroupMessages.go(5 hunks)contracts/pkg/identityupdates/IdentityUpdates.go(5 hunks)contracts/pkg/ratesmanager/RatesManager.go(4 hunks)pkg/server/server_test.go(3 hunks)pkg/testutils/anvil/anvil.go(1 hunks)pkg/testutils/network/port.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/testutils/anvil/anvil.go
🔇 Additional comments (17)
contracts/pkg/ratesmanager/RatesManager.go (3)
42-43: Updated ABI and Bin to include the new PAGE_SIZE method.The contract metadata has been updated with a new
PAGE_SIZEfunction and modified return type specifications forgetRatesCount, along with the corresponding binary updates. These changes are automatically generated and reflect the updated contract interface.
244-273: Added PAGE_SIZE accessor methods to the contract bindings.Three new Go methods have been added to access the
PAGE_SIZEconstant from the smart contract:
- For direct calling via
RatesManagerCaller- For session-based calling via
RatesManagerSession- For caller session-based calling via
RatesManagerCallerSessionThese follow the standard go-ethereum binding pattern and correctly map to the Solidity function.
384-384: Updated function signature comments for getRatesCount.The function signature in the comments has been updated to include the named return parameter
countfor clarity. This matches the modified contract ABI and improves documentation.Also applies to: 401-401, 408-408
pkg/testutils/network/port.go (1)
10-15: Well-implemented utility function for finding available ports.The implementation correctly uses the OS port assignment mechanism and properly handles resource cleanup with deferred close. This is a clean, reusable utility that will help avoid port conflicts in tests.
pkg/server/server_test.go (3)
24-24: Good addition of the network test utilities import.This import properly integrates the new port finding utility.
78-82: Good refactoring to use the shared port utility.Replacing the local port finding mechanism with the shared utility eliminates code duplication and improves maintainability.
222-223: Consistent application of the refactoring.The port utility is consistently used across all test cases, ensuring uniform behavior.
contracts/pkg/identityupdates/IdentityUpdates.go (5)
33-35: ABI updated to include absolute payload size constants.The contract ABI has been updated to include two new view functions:
ABSOLUTE_MAX_PAYLOAD_SIZEandABSOLUTE_MIN_PAYLOAD_SIZE, which provide fixed constants for payload size limits. These complement the existingmaxPayloadSizeandminPayloadSizefunctions that return configurable values.
205-220: Properly implemented Go binding for ABSOLUTEMAXPAYLOADSIZE.The Go binding correctly implements the Solidity
ABSOLUTE_MAX_PAYLOAD_SIZEfunction asABSOLUTEMAXPAYLOADSIZEfollowing Go naming conventions. The function is properly defined with appropriate error handling and type conversion.
222-234: Session wrapper methods added for consistency.The Session and CallerSession wrapper methods are correctly implemented, maintaining consistency with the rest of the binding interface. This allows the function to be called from different contexts (direct caller, session with default options, caller session).
236-265: ABSOLUTEMINPAYLOADSIZE implementation follows the same pattern.The implementation of
ABSOLUTEMINPAYLOADSIZEfollows the same pattern asABSOLUTEMAXPAYLOADSIZE, with proper error handling and type conversion, as well as appropriate session wrappers.
393-393: Updated comments on existing size methods.The comments for the existing
maxPayloadSizeandminPayloadSizemethods have been updated to indicate they return auint256 size. This clarifies the distinction between these configurable methods and the new absolute constants.Also applies to: 410-410, 424-424, 441-441, 448-448
contracts/pkg/groupmessages/GroupMessages.go (5)
34-35: ABI and binary updates correctly reflect the contract changes.The ABI string now includes the new
ABSOLUTE_MAX_PAYLOAD_SIZEandABSOLUTE_MIN_PAYLOAD_SIZEfunctions, and the binary has been updated accordingly. These changes are consistent with the addition of the new contract methods.
205-234: Implementation of ABSOLUTEMAXPAYLOADSIZE follows correct binding pattern.The new function properly implements the binding to the contract method "ABSOLUTE_MAX_PAYLOAD_SIZE" with appropriate implementations for Caller, Session, and CallerSession contexts, following the established pattern in this codebase.
236-265: Implementation of ABSOLUTEMINPAYLOADSIZE follows correct binding pattern.The new function properly implements the binding to the contract method "ABSOLUTE_MIN_PAYLOAD_SIZE" with appropriate implementations for Caller, Session, and CallerSession contexts, maintaining consistency with other function implementations.
393-393: Updated comment clarifies return value for maxPayloadSize.The comment has been improved to specify that the function returns a size value, enhancing code documentation.
Also applies to: 410-410, 417-417
424-424: Updated comment clarifies return value for minPayloadSize.The comment has been improved to specify that the function returns a size value, enhancing code documentation.
Also applies to: 441-441, 448-448
…b.com:xmtp/xmtpd into 03-13-test_utility_to_start_an_anvil_instance
### TL;DR Refactored blockchain tests to use Anvil for isolated test environments ## Issues #630 ### What changed? - Modified blockchain test files to use isolated Anvil instances for each test - Updated contract deployment functions to accept an RPC URL parameter - Added proper cleanup of Anvil instances after tests complete - Added a `Contract()` accessor method to `RatesAdmin` to expose the underlying contract - Moved blockchain package tests to `blockchain_test` package for better isolation - Replaced `defer cleanup()` with `t.Cleanup(cleanup)` in some tests - Added `NewContractsOptions` function that accepts an RPC URL parameter - Fixed initialization of deployed contracts in test utilities ### How to test? Run the blockchain tests: ``` go test ./pkg/blockchain/... -v ``` ### Why make this change? This change improves test reliability by ensuring each test runs in an isolated blockchain environment. By using separate Anvil instances for each test, we eliminate potential interference between tests and make them more deterministic. The code also properly cleans up resources after tests complete, preventing resource leaks. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new administrative interface that enables direct access to rate management functionality. - **Quality Improvements** - Upgraded blockchain integration and configuration to support dynamic contract deployments. - Streamlined resource cleanup and testing processes to enhance system stability and reliability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…b.com:xmtp/xmtpd into 03-13-test_utility_to_start_an_anvil_instance
TL;DR
Added a utility package for starting and managing Anvil Ethereum test nodes in tests.
Issues
#630
What changed?
anvilpackage underpkg/testutilswith functions to:How to test?
Run the tests in the new package:
The tests will verify that Anvil can be started, connected to, and that multiple instances can run concurrently.
Why make this change?
This utility simplifies testing code that interacts with Ethereum by providing a consistent way to spin up local Anvil instances. It handles port allocation, startup synchronization, and proper cleanup, making blockchain-dependent tests more reliable and easier to write.
Summary by CodeRabbit
New Features
RatesManagerinstance within theRatesAdminstruct.GroupMessagesandIdentityUpdatescontracts.Tests
GroupMessagesandIdentityUpdatescontracts, enhancing clarity and robustness.RatesManagercontract to cover new functionalities and edge cases.