feat: [E2E] Layer 1 — stack interfaces, types, compose override, Makefile targets#186
feat: [E2E] Layer 1 — stack interfaces, types, compose override, Makefile targets#186sadiq1971 wants to merge 1 commit intofeat/e2e-initfrom
Conversation
…file targets (#178) - Add tests/e2e/devstack/stack/interfaces.go: Anvil, Canton, APIServer, Relayer, Indexer, Postgres interfaces (build tag: e2e) - Add tests/e2e/devstack/stack/types.go: ServiceManifest, Account, AnvilAccount0/1, all request/response/row types, indexer page types - Add tests/e2e/docker-compose.e2e.yaml: thin include wrapper over root docker-compose.yaml; single entry point for E2E test runs - Extend Makefile with test-e2e, test-e2e-api, test-e2e-bridge, test-e2e-indexer targets (stub: "not yet implemented")
There was a problem hiding this comment.
Code Review
This pull request establishes the infrastructure for end-to-end testing by introducing new Makefile targets, a dedicated Docker Compose configuration, and Go interfaces and types representing the various components of the stack. The reviewer provided several constructive suggestions to improve the robustness and clarity of the test code, including renaming the generic Postgres interface to APIDatabase, expanding the ServiceManifest to include connection strings for all databases in the stack, utilizing constants for event types instead of magic strings, and defining preconfigured test accounts as values to prevent accidental mutation during tests.
| type Postgres interface { | ||
| // DSN returns the postgres connection string. | ||
| DSN() string | ||
| // WhitelistAddress adds an EVM address to the whitelist table. | ||
| WhitelistAddress(ctx context.Context, evmAddress string) error | ||
| // GetUserByEVMAddress returns a user row or nil. | ||
| GetUserByEVMAddress(ctx context.Context, evmAddress string) (*UserRow, error) | ||
| } |
There was a problem hiding this comment.
The Postgres interface name is too generic given that its methods (WhitelistAddress, GetUserByEVMAddress) are specifically tailored to the API server's user schema. Since the stack includes multiple databases (Relayer, API Server, Indexer), this interface should be renamed to something more specific like APIDatabase to avoid confusion and allow for other database-specific interfaces to be added later.
| type Postgres interface { | |
| // DSN returns the postgres connection string. | |
| DSN() string | |
| // WhitelistAddress adds an EVM address to the whitelist table. | |
| WhitelistAddress(ctx context.Context, evmAddress string) error | |
| // GetUserByEVMAddress returns a user row or nil. | |
| GetUserByEVMAddress(ctx context.Context, evmAddress string) (*UserRow, error) | |
| } | |
| type APIDatabase interface { | |
| // DSN returns the postgres connection string. | |
| DSN() string | |
| // WhitelistAddress adds an EVM address to the whitelist table. | |
| WhitelistAddress(ctx context.Context, evmAddress string) error | |
| // GetUserByEVMAddress returns a user row or nil. | |
| GetUserByEVMAddress(ctx context.Context, evmAddress string) (*UserRow, error) | |
| } |
| RelayerHTTP string // "http://localhost:8080" | ||
| IndexerHTTP string // "http://localhost:8082" | ||
| OAuthHTTP string // "http://localhost:8088" | ||
| PostgresDSN string // "postgres://postgres:p@ssw0rd@localhost:5432/erc20_api" |
There was a problem hiding this comment.
The ServiceManifest currently only provides a single PostgresDSN which points to the erc20_api database. However, the docker-compose.yaml defines three distinct databases: relayer, erc20_api, and canton_indexer. E2E tests may need to verify state across all components. Consider adding DSN fields for the Relayer and Indexer databases as well.
| PostgresDSN string // "postgres://postgres:p@ssw0rd@localhost:5432/erc20_api" | |
| APIDatabaseDSN string // "postgres://postgres:p@ssw0rd@localhost:5432/erc20_api" | |
| RelayerDatabaseDSN string // "postgres://postgres:p@ssw0rd@localhost:5432/relayer" | |
| IndexerDatabaseDSN string // "postgres://postgres:p@ssw0rd@localhost:5432/canton_indexer" |
| // IndexerEvent mirrors indexer.ParsedEvent — a decoded TokenTransferEvent from the ledger. | ||
| type IndexerEvent struct { | ||
| ContractID string `json:"contract_id"` | ||
| TxID string `json:"tx_id"` | ||
| InstrumentAdmin string `json:"instrument_admin"` | ||
| InstrumentID string `json:"instrument_id"` | ||
| EventType string `json:"event_type"` // "MINT" | "BURN" | "TRANSFER" |
There was a problem hiding this comment.
Using magic strings for EventType in tests can lead to typos and maintenance issues. It is recommended to define these as constants.
| // IndexerEvent mirrors indexer.ParsedEvent — a decoded TokenTransferEvent from the ledger. | |
| type IndexerEvent struct { | |
| ContractID string `json:"contract_id"` | |
| TxID string `json:"tx_id"` | |
| InstrumentAdmin string `json:"instrument_admin"` | |
| InstrumentID string `json:"instrument_id"` | |
| EventType string `json:"event_type"` // "MINT" | "BURN" | "TRANSFER" | |
| const ( | |
| EventTypeMint = "MINT" | |
| EventTypeBurn = "BURN" | |
| EventTypeTransfer = "TRANSFER" | |
| ) | |
| // IndexerEvent mirrors indexer.ParsedEvent — a decoded TokenTransferEvent from the ledger. | |
| type IndexerEvent struct { | |
| ContractID string `json:"contract_id"` | |
| TxID string `json:"tx_id"` | |
| InstrumentAdmin string `json:"instrument_admin"` | |
| InstrumentID string `json:"instrument_id"` | |
| EventType string `json:"event_type"` // "MINT" | "BURN" | "TRANSFER" |
| // Preconfigured Anvil test accounts (deterministic from mnemonic). | ||
| var ( | ||
| AnvilAccount0 = &Account{ | ||
| Address: common.HexToAddress("0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266"), | ||
| PrivateKey: "ac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80", | ||
| } | ||
| AnvilAccount1 = &Account{ | ||
| Address: common.HexToAddress("0x70997970C51812dc3A010C7d01b50e0d17dc79C8"), | ||
| PrivateKey: "59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d", | ||
| } | ||
| ) |
There was a problem hiding this comment.
Exporting preconfigured test accounts as global pointers allows any test to accidentally mutate their fields (e.g., changing the PrivateKey), which can lead to flaky tests and hard-to-debug side effects. It is safer to define them as values or provide them via a function that returns a fresh copy.
| // Preconfigured Anvil test accounts (deterministic from mnemonic). | |
| var ( | |
| AnvilAccount0 = &Account{ | |
| Address: common.HexToAddress("0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266"), | |
| PrivateKey: "ac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80", | |
| } | |
| AnvilAccount1 = &Account{ | |
| Address: common.HexToAddress("0x70997970C51812dc3A010C7d01b50e0d17dc79C8"), | |
| PrivateKey: "59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d", | |
| } | |
| ) | |
| // Preconfigured Anvil test accounts (deterministic from mnemonic). | |
| var ( | |
| AnvilAccount0 = Account{ | |
| Address: common.HexToAddress("0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266"), | |
| PrivateKey: "ac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80", | |
| } | |
| AnvilAccount1 = Account{ | |
| Address: common.HexToAddress("0x70997970C51812dc3A010C7d01b50e0d17dc79C8"), | |
| PrivateKey: "59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d", | |
| } | |
| ) |
Summary
tests/e2e/devstack/stack/interfaces.go—Anvil,Canton,APIServer,Relayer,Indexer,Postgresinterfaces; all test and DSL code depends only on these, never on concrete implementations (//go:build e2e)tests/e2e/devstack/stack/types.go—ServiceManifest,Account,AnvilAccount0/1, request/response/row types, and all indexer paginated page typestests/e2e/docker-compose.e2e.yaml— thinincludewrapper over the rootdocker-compose.yaml; single entry point fordocker compose up --waitin E2E runsMakefile—test-e2e,test-e2e-api,test-e2e-bridge,test-e2e-indexertargets (stubbed with "not yet implemented" until later layers land)Note on compose port overrides
The
includedirective does not support overriding services from the included file. Since the rootdocker-compose.yamlalready exposes all ports with fixed bindings (8545:8545,5011:5011, etc.) and thee2e-deployvolume was already wired in #177, no service overrides are needed —docker compose portresolves correctly against the fixed mappings.Acceptance Criteria
go build -tags e2e ./tests/e2e/devstack/stack/...compiles cleanlydocker compose -f tests/e2e/docker-compose.e2e.yaml configsucceedsmake test-e2e-api,make test-e2e-bridge,make test-e2e-indexereach print "not yet implemented"Closes #178