[IND-510] containerized env for entire network #823
Conversation
WalkthroughThe changes involve the introduction of new environment variables, configuration files, and testing utilities across various services within an indexer project. These updates include database and Kafka configurations, Ethereum RPC endpoint adjustments, and the setup of a new fixed price exchange for testing purposes. The modifications suggest a significant enhancement in the project's testing infrastructure, particularly for end-to-end (E2E) testing, and the integration of new service components. Changes
TipsChat with CodeRabbit Bot (
|
dydxwill
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (6)
- indexer/docker-compose-e2e-test.yml
- indexer/docker-compose-local-deployment.yml
- indexer/pnpm-lock.yaml
- indexer/services/e2e-testing/package.json
- indexer/services/e2e-testing/tsconfig.eslint.json
- indexer/services/e2e-testing/tsconfig.json
Files selected for processing (10)
- indexer/services/e2e-testing/.env (1 hunks)
- indexer/services/e2e-testing/.env.test (1 hunks)
- indexer/services/e2e-testing/.eslintrc.js (1 hunks)
- indexer/services/e2e-testing/README.md (1 hunks)
- indexer/services/e2e-testing/tests/index.test.ts (1 hunks)
- indexer/services/e2e-testing/jest.config.js (1 hunks)
- indexer/services/e2e-testing/jest.globalSetup.js (1 hunks)
- indexer/services/e2e-testing/jest.setup.js (1 hunks)
- indexer/services/e2e-testing/src/config.ts (1 hunks)
- indexer/services/e2e-testing/src/index.ts (1 hunks)
Files skipped from review due to trivial changes (4)
- indexer/services/e2e-testing/.env
- indexer/services/e2e-testing/README.md
- indexer/services/e2e-testing/jest.setup.js
- indexer/services/e2e-testing/src/index.ts
Additional comments: 5
indexer/services/e2e-testing/.env.test (1)
- 3-6: The environment variables for the test database are set correctly for a test environment. However, ensure that the hardcoded password is secure and that the database is not accessible from outside the test environment to prevent any security risks. Additionally, verify that the non-standard port number
5436does not conflict with other services.indexer/services/e2e-testing/.eslintrc.js (1)
- 1-11: The ESLint configuration correctly extends the base configuration and sets the
tsconfigRootDirto__dirname, ensuring that ESLint uses the correct TypeScript configuration for the project.indexer/services/e2e-testing/jest.config.js (1)
- 1-2: The changes to
jest.config.jslook good, as it now correctly requires a shared Jest configuration from the@dydxprotocol-indexer/devpackage. This should help maintain consistency across projects.indexer/services/e2e-testing/jest.globalSetup.js (1)
- 1-6: The use of
dotenv-flowto load environment variables is a standard practice for managing different configurations for testing environments. Ensure that the.envfiles are correctly named and placed in the project structure so thatdotenv-flowcan pick them up as expected. The ESLint disable comment is justified in this context as dynamic requires are necessary fordotenv-flowto function properly.indexer/services/e2e-testing/src/config.ts (1)
- 1-11: The changes consolidate configuration schemas from different services and parse them for use, which aligns with the PR's objective to containerize the environment for the entire network. Ensure that the dependent services correctly import and use the new consolidated configuration.
| describe('index', () => { | ||
| it('true is true', () => { | ||
| expect(true).toEqual(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test case 'true is true' does not test any functionality. Consider expanding this test to cover actual features or mark it as a TODO for future development.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- indexer/docker-compose-e2e-test.yml
Files selected for processing (3)
- indexer/services/e2e-testing/.env.development (1 hunks)
- indexer/services/e2e-testing/.env.test (1 hunks)
- indexer/services/e2e-testing/tests/index.test.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- indexer/services/e2e-testing/.env.test
Additional comments: 1
indexer/services/e2e-testing/.env.development (1)
- 12-12: The
KAFKA_BROKER_URLSvariable is set tokafka:9092, which seems to be a service name and port within a Docker network or similar containerized environment. Verify that this service name is correctly resolved in the containerized network setup.
| DB_NAME=dydx_dev | ||
| DB_USERNAME=dydx_dev | ||
| DB_PASSWORD=dydxserver123 | ||
| PG_POOL_MAX=2 | ||
| PG_POOL_MIN=1 | ||
| DB_HOSTNAME=localhost | ||
| DB_READONLY_HOSTNAME=localhost | ||
| DB_PORT=5435 |
There was a problem hiding this comment.
Ensure that sensitive information such as DB_PASSWORD is not hardcoded in the environment file and is instead securely managed, for example, through a secrets management system.
| export const DYDX_LOCAL_ADDRESS = 'dydx199tqg4wdlnu4qjlxchpd7seg454937hjrknju4'; | ||
| export const DYDX_LOCAL_MNEMONIC = 'merge panther lobster crazy road hollow amused security before critic about cliff exhibit cause coyote talent happy where lion river tobacco option coconut small'; |
There was a problem hiding this comment.
Hardcoded sensitive information should be avoided. If this mnemonic is used for testing purposes only, consider moving it to a configuration file or using environment variables.
| describe('index', () => { | ||
| it('test transfers', async () => { | ||
| const wallet = await LocalWallet.fromMnemonic(DYDX_LOCAL_MNEMONIC, BECH32_PREFIX); | ||
| // console.log(wallet); |
There was a problem hiding this comment.
Remove commented-out console logs to keep the code clean and maintainable.
Also applies to: 27-28, 36-37
| it('test transfers', async () => { | ||
| const wallet = await LocalWallet.fromMnemonic(DYDX_LOCAL_MNEMONIC, BECH32_PREFIX); | ||
| // console.log(wallet); | ||
|
|
||
| const client = await ValidatorClient.connect(Network.local().validatorConfig); | ||
| // console.log('**Client**'); | ||
| // console.log(client); | ||
|
|
||
| const subaccount = new SubaccountInfo(wallet, 0); | ||
| const tx = await client.post.deposit( | ||
| subaccount, | ||
| 0, | ||
| new Long(10_000_000), | ||
| ); | ||
| // console.log('**Deposit Tx**'); | ||
| // console.log(tx); | ||
|
|
||
| const defaultSubaccountId: string = SubaccountTable.uuid(wallet.address!, 0); | ||
| const transfers: TransferFromDatabase[] = await TransferTable.findAllToOrFromSubaccountId( | ||
| { subaccountId: [defaultSubaccountId] }, | ||
| [], { | ||
| orderBy: [[TransferColumns.id, Ordering.ASC]], | ||
| }); | ||
|
|
||
| expect(transfers.length).toBeGreaterThan(0); | ||
| }); |
There was a problem hiding this comment.
The test case should mock external dependencies to avoid flakiness and ensure the test is reliable regardless of external service status.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- e2e-testing/docker-compose-e2e-test.yml
Files selected for processing (12)
- e2e-testing/.env (1 hunks)
- e2e-testing/run-containerized-env.sh (1 hunks)
- protocol/Makefile (1 hunks)
- protocol/daemons/pricefeed/client/constants/exchange_common/exchange_id.go (1 hunks)
- protocol/daemons/pricefeed/client/constants/static_exchange_details.go (2 hunks)
- protocol/daemons/pricefeed/client/constants/static_exchange_query_config.go (1 hunks)
- protocol/daemons/pricefeed/client/price_function/test_fixed_price_exchange/exchange_query_details.go (1 hunks)
- protocol/daemons/pricefeed/client/price_function/test_fixed_price_exchange/fixed_price_function.go (1 hunks)
- protocol/testing/e2etest-local/Dockerfile (1 hunks)
- protocol/testing/e2etest-local/local.sh (1 hunks)
- protocol/testing/genesis.sh (1 hunks)
- protocol/testutil/daemons/pricefeed/exchange_config/testnet_exchange_market_config.go (1 hunks)
Files skipped from review due to trivial changes (6)
- e2e-testing/.env
- e2e-testing/run-containerized-env.sh
- protocol/daemons/pricefeed/client/constants/exchange_common/exchange_id.go
- protocol/daemons/pricefeed/client/constants/static_exchange_details.go
- protocol/testing/e2etest-local/Dockerfile
- protocol/testutil/daemons/pricefeed/exchange_config/testnet_exchange_market_config.go
Additional comments: 10
protocol/Makefile (2)
376-378: The new target
build-e2etest-imageis correctly defined to build an end-to-end test image.383-383: The target
e2etest-build-imagecorrectly depends onlocalnet-initandbuild-e2etest-image.protocol/daemons/pricefeed/client/constants/static_exchange_query_config.go (1)
- 130-135: The addition of
EXCHANGE_ID_TEST_FIXED_PRICE_EXCHANGEtoStaticExchangeQueryConfigis consistent with the PR's objective to introduce a containerized environment and the associated changes to the network, including the introduction of a new test fixed price exchange. This change should be cross-verified with the rest of the codebase to ensure that the new exchange ID is properly integrated and handled whereverStaticExchangeQueryConfigis used.protocol/daemons/pricefeed/client/price_function/test_fixed_price_exchange/exchange_query_details.go (2)
29-29: The URL "https://jsonplaceholder.typicode.com/users" appears to be a placeholder. Please verify if this is intended for testing purposes or if it should be replaced with the actual endpoint.
30-30: The
FixedExchangePriceFunctionis referenced here. Ensure that this function is defined and implemented correctly elsewhere in the codebase.protocol/daemons/pricefeed/client/price_function/test_fixed_price_exchange/fixed_price_function.go (3)
11-32: The
FixedPriceTickerstruct and its methods are correctly implemented and adhere to theprice_function.Tickerinterface.39-55: The
FixedExchangePriceFunctionusesfmt.Sprintfto convert float values to strings for thePricefield inFixedPriceTicker. Ensure that the string representation of the price is handled correctly in all places where it is used.34-56: The
FixedExchangePriceFunctionis correctly implemented to return median prices from a set of fixed price tickers.protocol/testing/genesis.sh (2)
1575-1604: The addition of the
update_all_markets_with_fixed_price_exchangefunction in thegenesis.shscript aligns with the PR objectives and the AI-generated summaries. This function will modify the genesis file to use a fixed price exchange for each market, which is a significant change to the market configuration within the genesis file.1606-1608: Please verify the implementation of the
update_genesis_complete_bridge_delayfunction, as it is not included within the provided hunk. Ensure that it correctly updates thecomplete_bridge_delayparameter in the genesis file as intended.
| create_validators() { | ||
| # Create temporary directory for all gentx files. | ||
| mkdir /tmp/gentx | ||
|
|
||
| # Iterate over all validators and set up their home directories, as well as generate `gentx` transaction for each. | ||
| for i in "${!MONIKERS[@]}"; do | ||
| VAL_HOME_DIR="$HOME/chain/.${MONIKERS[$i]}" | ||
| VAL_CONFIG_DIR="$VAL_HOME_DIR/config" | ||
|
|
||
| # Initialize the chain and validator files. | ||
| dydxprotocold init "${MONIKERS[$i]}" -o --chain-id=$CHAIN_ID --home "$VAL_HOME_DIR" | ||
|
|
||
| # Overwrite the randomly generated `priv_validator_key.json` with a key generated deterministically from the mnemonic. | ||
| dydxprotocold tendermint gen-priv-key --home "$VAL_HOME_DIR" --mnemonic "${MNEMONICS[$i]}" | ||
|
|
||
| # Note: `dydxprotocold init` non-deterministically creates `node_id.json` for each validator. | ||
| # This is inconvenient for persistent peering during testing in Terraform configuration as the `node_id` | ||
| # would change with every build of this container. | ||
| # | ||
| # For that reason we overwrite the non-deterministically generated one with a deterministic key defined in this file here. | ||
| new_file=$(jq ".priv_key.value = \"${NODE_KEYS[$i]}\"" "$VAL_CONFIG_DIR"/node_key.json) | ||
| cat <<<"$new_file" >"$VAL_CONFIG_DIR"/node_key.json | ||
|
|
||
| edit_config "$VAL_CONFIG_DIR" | ||
|
|
||
| # Using "*" as a subscript results in a single arg: "dydx1... dydx1... dydx1..." | ||
| # Using "@" as a subscript results in separate args: "dydx1..." "dydx1..." "dydx1..." | ||
| # Note: `edit_genesis` must be called before `add-genesis-account`. | ||
| edit_genesis "$VAL_CONFIG_DIR" "${TEST_ACCOUNTS[*]}" "${FAUCET_ACCOUNTS[*]}" "" "" "" "" | ||
| update_genesis_use_test_volatile_market "$VAL_CONFIG_DIR" | ||
| update_all_markets_with_fixed_price_exchange "$VAL_CONFIG_DIR" | ||
| update_genesis_complete_bridge_delay "$VAL_CONFIG_DIR" "30" | ||
|
|
||
| echo "${MNEMONICS[$i]}" | dydxprotocold keys add "${MONIKERS[$i]}" --recover --keyring-backend=test --home "$VAL_HOME_DIR" | ||
|
|
||
| for acct in "${TEST_ACCOUNTS[@]}"; do | ||
| dydxprotocold add-genesis-account "$acct" 100000000000000000$USDC_DENOM,$TESTNET_VALIDATOR_NATIVE_TOKEN_BALANCE$NATIVE_TOKEN --home "$VAL_HOME_DIR" | ||
| done | ||
| for acct in "${FAUCET_ACCOUNTS[@]}"; do | ||
| dydxprotocold add-genesis-account "$acct" 900000000000000000$USDC_DENOM,$TESTNET_VALIDATOR_NATIVE_TOKEN_BALANCE$NATIVE_TOKEN --home "$VAL_HOME_DIR" | ||
| done | ||
|
|
||
| dydxprotocold gentx "${MONIKERS[$i]}" $TESTNET_VALIDATOR_SELF_DELEGATE_AMOUNT$NATIVE_TOKEN --moniker="${MONIKERS[$i]}" --keyring-backend=test --chain-id=$CHAIN_ID --home "$VAL_HOME_DIR" | ||
|
|
||
| # Copy the gentx to a shared directory. | ||
| cp -a "$VAL_CONFIG_DIR/gentx/." /tmp/gentx | ||
| done | ||
|
|
||
| # Copy gentxs to the first validator's home directory to build the genesis json file | ||
| FIRST_VAL_HOME_DIR="$HOME/chain/.${MONIKERS[0]}" | ||
| FIRST_VAL_CONFIG_DIR="$FIRST_VAL_HOME_DIR/config" | ||
|
|
||
| rm -rf "$FIRST_VAL_CONFIG_DIR/gentx" | ||
| mkdir "$FIRST_VAL_CONFIG_DIR/gentx" | ||
| cp -r /tmp/gentx "$FIRST_VAL_CONFIG_DIR" | ||
|
|
||
| # Build the final genesis.json file that all validators and the full-nodes will use. | ||
| dydxprotocold collect-gentxs --home "$FIRST_VAL_HOME_DIR" | ||
|
|
||
| # Copy this genesis file to each of the other validators | ||
| for i in "${!MONIKERS[@]}"; do | ||
| if [[ "$i" == 0 ]]; then | ||
| # Skip first moniker as it already has the correct genesis file. | ||
| continue | ||
| fi | ||
|
|
||
| VAL_HOME_DIR="$HOME/chain/.${MONIKERS[$i]}" | ||
| VAL_CONFIG_DIR="$VAL_HOME_DIR/config" | ||
| rm -rf "$VAL_CONFIG_DIR/genesis.json" | ||
| cp "$FIRST_VAL_CONFIG_DIR/genesis.json" "$VAL_CONFIG_DIR/genesis.json" | ||
| done | ||
| } | ||
|
|
||
| setup_cosmovisor() { | ||
| for i in "${!MONIKERS[@]}"; do | ||
| VAL_HOME_DIR="$HOME/chain/.${MONIKERS[$i]}" | ||
| export DAEMON_NAME=dydxprotocold | ||
| export DAEMON_HOME="$HOME/chain/.${MONIKERS[$i]}" | ||
|
|
||
| cosmovisor init /bin/dydxprotocold | ||
| done | ||
| } | ||
|
|
||
| # TODO(DEC-1894): remove this function once we migrate off of persistent peers. | ||
| # Note: DO NOT add more config modifications in this method. Use `cmd/config.go` to configure | ||
| # the default config values. | ||
| edit_config() { | ||
| CONFIG_FOLDER=$1 | ||
|
|
||
| # Disable pex | ||
| dasel put -t bool -f "$CONFIG_FOLDER"/config.toml '.p2p.pex' -v 'false' | ||
|
|
||
| # Default `timeout_commit` is 999ms. For local testnet, use a larger value to make | ||
| # block time longer for easier troubleshooting. | ||
| dasel put -t string -f "$CONFIG_FOLDER"/config.toml '.consensus.timeout_commit' -v '5s' | ||
| } | ||
|
|
||
| install_prerequisites | ||
| create_validators | ||
| setup_cosmovisor |
There was a problem hiding this comment.
The script local.sh appears to be well-structured and follows bash scripting conventions. It includes the creation of validators, setting up their home directories, and initializing the genesis files. The use of arrays to manage mnemonics, node keys, and monikers is a good practice for maintainability and readability. The script also includes a function to install prerequisites and setup cosmovisor, which is a good separation of concerns.
However, there are a few areas that could be improved or need attention:
-
The script assumes that it is being run in a specific directory structure (
$HOME/chain/.${MONIKERS[$i]}), which may not be the case in all environments. It would be beneficial to either document this requirement or make the script more flexible regarding the directory structure. -
The script uses hard-coded values for tokens and account balances (e.g.,
100000000000000000$USDC_DENOM,$TESTNET_VALIDATOR_NATIVE_TOKEN_BALANCE$NATIVE_TOKEN). It would be better to define these as variables at the top of the script or in a separate configuration file to make them easily configurable. -
The script does not include any error handling for the commands it runs. Adding error handling would make the script more robust and easier to debug if something goes wrong.
-
The script includes a TODO comment (
# TODO(DEC-1894): remove this function once we migrate off of persistent peers.) on line 157. This should be tracked in an issue tracker rather than in a comment to ensure it is not forgotten. -
The script uses
daselandjqfor JSON and YAML manipulation, which are not standard Unix tools and may not be present on all systems. It would be good to check for their presence and install them if necessary or document these dependencies. -
The script includes a
setup_cosmovisorfunction, but it is not clear from the script whatcosmovisoris or why it is needed. Adding comments to explain this would be helpful for maintainability.
Overall, the script is functional but could benefit from improvements in flexibility, configurability, error handling, and documentation.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/testutil/daemons/pricefeed/exchange_config/testnet_exchange_market_config.go (1 hunks)
Additional comments: 1
protocol/testutil/daemons/pricefeed/exchange_config/testnet_exchange_market_config.go (1)
- 1079-1092: The addition of
EXCHANGE_ID_TEST_FIXED_PRICE_EXCHANGEwith market configurations for Bitcoin, Ethereum, and Solana is consistent with the PR's objective to introduce a containerized environment for testing. This configuration will be used to simulate a fixed price exchange in a test environment, which is a valuable addition for testing price feed daemons.
…wl/qual_container
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/testing/e2etest-local/local.sh (1 hunks)
Additional comments: 3
protocol/testing/e2etest-local/local.sh (3)
4-5: The comment on lines 4-5 provides clear context for the purpose of the script, which is good for maintainability.
12-27: Ensure that the hard-coded mnemonics and node keys are used exclusively for testing and not in production environments to maintain security.
68-70: The
install_prerequisitesfunction ensures that necessary tools (daselandjq) are installed, which is good for ensuring the script's portability and reliability.
| # This file initializes muliple validators for local and CI testing purposes. | ||
| # This file should be run as part of `docker-compose-e2e-test.yml`. | ||
|
|
||
| source "./genesis.sh" |
There was a problem hiding this comment.
Consider checking if genesis.sh exists and is executable before sourcing it to prevent potential runtime errors.
if [ ! -f "./genesis.sh" ] || [ ! -x "./genesis.sh" ]; then
echo "Required file genesis.sh does not exist or is not executable."
exit 1
fi| # TODO(DEC-1894): remove this function once we migrate off of persistent peers. | ||
| # Note: DO NOT add more config modifications in this method. Use `cmd/config.go` to configure | ||
| # the default config values. |
There was a problem hiding this comment.
The TODO comment on lines 157-159 is a good reminder of technical debt, but ensure that it is also tracked in a project management tool for better visibility and tracking.
| # Initialize their genesis files and home directories. | ||
| create_validators() { | ||
| # Create temporary directory for all gentx files. | ||
| mkdir /tmp/gentx |
There was a problem hiding this comment.
Consider adding logic to clean up the temporary directory /tmp/gentx after its use to prevent potential resource waste and conflicts.
trap 'rm -rf /tmp/gentx' EXIT
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.