Use pre-baked image for all anvil-based tests #823
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 removes all test-time deployments of smart contracts from the codebase. Instead of deploying contracts dynamically in test helpers, fixed contract addresses are now provided via constants in the configuration. All deployment helper functions and their tests are deleted. The Anvil test node startup is refactored to use Docker containers for improved test isolation and reliability. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Config
participant Anvil (Docker)
Note over Test: Test setup (before)
Test->>Anvil (Docker): Start test node
Test->>Test: Deploy contract(s) via helper
Test->>Config: Set contract address in config
Test->>Test: Run test logic
Note over Test: Test setup (after)
Test->>Anvil (Docker): Start test node
Test->>Config: Use fixed contract address from config
Test->>Test: Run test logic
Possibly related PRs
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 Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ 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 (
|
Replace dynamic contract deployment with pre-baked Docker image in Anvil-based tests to improve test reliability
📍Where to StartStart with the Macroscope summarized 8f1c80d. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
pkg/testutils/config.go (2)
16-20: Address constants are fine, but flag static duplicationHard-coding contract addresses is reasonable for deterministic tests; however, we now have three places with private keys / addresses (
TEST_PRIVATE_KEYabove,LOCAL_PRIVATE_KEYincontracts.go, and these four constants). Consider keeping all blockchain test constants in a single file (or anaddresses_test.go) to avoid drift.🧰 Tools
🪛 GitHub Check: Lint-Go
[failure] 16-16:
File is not properly formatted (gofumpt)
24-37: Run gofumpt – file is not formatted
gofumptflagged this file. A quick pass will reorder the composite literal fields and trim the extra alignment whitespace. This prevents CI lint noise later.gofumpt -w pkg/testutils/config.gopkg/testutils/anvil/anvil_test.go (1)
5-5: Nit: keep imports gofumpt-sorted
fmtshould appear after the standard-librarycontextimport when usinggofumpt.pkg/testutils/anvil/anvil.go (2)
75-94: Redundant AutoRemove may race with explicitTerminate
SettingHostConfig.AutoRemove = truecauses Docker to delete the container as soon as it exits. Whent.Cleanuplater callsTerminate, the container may already be gone, making the call a no-op or returning a “No such container” error (currently ignored).If deterministic cleanup is important, either:
- Remove
AutoRemoveand rely solely onTerminate, or- Keep
AutoRemovebut drop the explicitTerminate, or- Keep both but check and deliberately ignore
errNotFoundin the cleanup for clarity.This avoids hard-to-trace test flakiness on slow or overloaded runners.
81-86: Considerwait.ForListeningPortinstead of polling client
Testcontainers already offerswait.ForListeningPort("8545/tcp"), which would allow you to delete the customwaitForAnvilfunction and its busy-wait loop. This reduces code and potential connection-race issues.Example:
-WaitingFor: wait.ForLog("Listening on"), +WaitingFor: wait.ForListeningPort("8545/tcp"),…then you can safely remove
waitForAnviland the extra dial logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
pkg/blockchain/blockchainPublisher_test.go(0 hunks)pkg/blockchain/migrator/migrator_test.go(0 hunks)pkg/blockchain/ratesAdmin_test.go(0 hunks)pkg/blockchain/registryAdmin_test.go(0 hunks)pkg/indexer/app_chain/contracts/group_message_storer_test.go(0 hunks)pkg/indexer/app_chain/contracts/identity_update_storer_test.go(0 hunks)pkg/indexer/e2e_test.go(0 hunks)pkg/testutils/anvil/anvil.go(2 hunks)pkg/testutils/anvil/anvil_test.go(2 hunks)pkg/testutils/config.go(1 hunks)pkg/testutils/contracts.go(1 hunks)pkg/testutils/contracts_test.go(0 hunks)
💤 Files with no reviewable changes (8)
- pkg/blockchain/blockchainPublisher_test.go
- pkg/indexer/e2e_test.go
- pkg/blockchain/migrator/migrator_test.go
- pkg/indexer/app_chain/contracts/identity_update_storer_test.go
- pkg/blockchain/ratesAdmin_test.go
- pkg/blockchain/registryAdmin_test.go
- pkg/indexer/app_chain/contracts/group_message_storer_test.go
- pkg/testutils/contracts_test.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
pkg/testutils/anvil/anvil_test.go (2)
pkg/testutils/anvil/anvil.go (1)
StartAnvil(69-103)pkg/blockchain/client.go (1)
NewClient(17-19)
pkg/testutils/config.go (1)
pkg/config/options.go (3)
ContractsOptions(16-32)AppChainOptions(34-40)SettlementChainOptions(42-49)
🪛 GitHub Check: Lint-Go
pkg/testutils/config.go
[failure] 16-16:
File is not properly formatted (gofumpt)
pkg/testutils/anvil/anvil.go
[failure] 6-6:
File is not properly formatted (gofumpt)
🪛 GitHub Actions: Lint
pkg/testutils/anvil/anvil.go
[error] 6-6: File is not properly formatted (gofumpt)
🪛 Gitleaks (8.26.0)
pkg/testutils/contracts.go
16-16: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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: Upgrade Tests
🔇 Additional comments (3)
pkg/testutils/contracts.go (1)
12-13:testingimport is fine, but make it explicit in build tagsNothing blocking here; just confirming the extra import is required for helper assertions below.
pkg/testutils/anvil/anvil_test.go (1)
27-41: Concurrency looks good – verify container limitsSpinning up 10 Anvil containers concurrently is useful for stress-testing but may overwhelm CI runners with low CPU/memory quotas. If jobs start to flake, consider reading
ANVIL_PARALLELISMfrom the environment to cap the number of sub-tests run in parallel.pkg/testutils/anvil/anvil.go (1)
70-71:t.Context()requires Go 1.22 – verify tool-chain compatibility
testing.T.Context()was introduced very recently (Go 1.22). If your CI/build image or downstream users are still on ≤ 1.21, the project will fail to compile. Consider:-ctx := t.Context() +ctx, cancel := context.WithCancel(context.Background()) +t.Cleanup(cancel)or bump the minimum Go version in
go.mod(and CI) to 1.22+.
| const ( | ||
| RATE_REGISTRY_ADDRESS = "0xE71ac6dE80392495eB52FB1dCa321f5dB8f51BAE" | ||
| NODE_REGISTRY_ADDRESS = "0x8d69E9834f1e4b38443C638956F7D81CD04eBB2F" | ||
| GROUP_MESSAGE_BROADCAST_ADDRESS = "0xD5b7B43B0e31112fF99Bd5d5C4f6b828259bedDE" | ||
| IDENTITY_UPDATE_BROADCAST_ADDRESS = "0xe67104BC93003192ab78B797d120DBA6e9Ff4928" | ||
| ) |
There was a problem hiding this comment.
we will have to figure out a better way to fetch these. Same for hardcoding the version of the contracts image. I will give it some thought and can create a followup PR
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/testutils/contracts.go (1)
16-18: Hardcoded private key remains a security concernThe
LOCAL_PRIVATE_KEYconstant is still hardcoded in the code, which triggers security scanning tools like gitleaks and could lead to accidental exposure or misuse. This issue was previously identified in a prior review.As suggested in the previous review, consider loading this key from an environment variable with a fallback:
-const ( - LOCAL_PRIVATE_KEY = "ac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80" -) +// Anvil image pre-funded key. Override with XMTPD_TEST_PRIVATE_KEY if needed. +var LOCAL_PRIVATE_KEY = func() string { + if v := os.Getenv("XMTPD_TEST_PRIVATE_KEY"); v != "" { + return v + } + return "ac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80" +}()Don't forget to add the missing import:
import "os"
🧹 Nitpick comments (1)
pkg/testutils/contracts.go (1)
16-18: Add documentation about key usage with pre-baked imageSince this PR shifts testing to use a pre-baked Docker image instead of dynamic contract deployments, it would be helpful to document that this private key corresponds to a pre-funded account in the Docker image.
const ( + // Account pre-funded in the ghcr.io/xmtp/contracts:sha-cff29fc Docker image + // Used for testing with the pre-baked Anvil blockchain LOCAL_PRIVATE_KEY = "ac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/testutils/anvil/anvil.go(2 hunks)pkg/testutils/config.go(1 hunks)pkg/testutils/contracts.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/testutils/config.go
🚧 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 (2)
- GitHub Check: Upgrade Tests
- GitHub Check: Test (Node)
Replace dynamic contract deployments with pre-baked Docker image to speed up anvil-based blockchain tests
Replaces the test environment setup across multiple test files by removing dynamic contract deployments and introducing a Docker-based approach using
testcontainers-go. The changes center around the anvil.go file which now uses a pre-configured Docker imageghcr.io/xmtp/contracts:sha-cff29fccontaining pre-deployed contracts. Key changes include:StartAnvilfunction to use Docker containers instead of direct anvil process executionNewContractsOptionsinstead of deploying new contracts📍Where to Start
Start with the
StartAnvilfunction in anvil.go which contains the core changes for the Docker-based test environment setup.Macroscope summarized 16070ed.
Summary by CodeRabbit
Refactor
Chores
Tests