refactor environment variables handling and remove unused code

[0xjunha]

Description

• common.py: removed default INDEXER_URL value since it shouldn't be set when we're running localnet without an indexer.

• integration_test.py: decoupled from common.py since currently it is also loaded in each test case, but only imported once at import time of integration_test.py. This nullifies environment variable handling in the setUpClass method.

• multikey.py: removed unused code

• README.md: APTOS_CORE_REPO => APTOS_CORE_PATH

Test Plan

Related Links

[davidiw]

Thanks!

We use indexer here: https://github.com/aptos-labs/aptos-python-sdk/blob/main/examples/transfer_coin.py#L68

better to keep it around since we likely will use it for some SDK Api, sooner than later.

[0xjunha]

@davidiw Got it!
Yes, the SDK should cover the following cases for local integration tests:

1. APTOS_CLI_PATH, APTOS_CORE_REPO set, and make integration_test will automatically bootstrap a localnet, and setup node/faucet urls in setUpClass of integration_test.py. However this currently doesn't work and the integration tests run on devnet as set in common.py as default values, this is why I decoupled common.py from integration_test.py. I think once the common.py is imported in integration_test.py, it is cached and each test case doesn't import it again.

   integration_test.py: decoupled from common.py since currently it is also loaded in each test case, but only imported once at import time of integration_test.py. This nullifies environment variable handling in the setUpClass method.

2. User runs a localnet manually with an indexer and APTOS_INDEXER_URL is set to http://127.0.0.1:8090/v1/graphql

   • Actually we should remove os.environ["APTOS_INDEXER_URL"] = "none" from setUpClass
     because it is currently overwriting the indexer url to none, not utilizing the local indexer even it is running.
   • (Added another commit below to remove this)

3. User runs a localnet manually without an indexer and APTOS_INDEXER_URL is not set

   • applying 2), we should keep the default value of INDEXER_URL of common.py removed since with the default value it will look into the devnet indexer, failing the integration test.

All the cases are handled properly in https://github.com/aptos-labs/aptos-python-sdk/blob/main/examples/transfer_coin.py#L16

This will still allow utilizing the indexer in local/devnet/testnet tests once the APTOS_INDEXER_URL is set properly

[davidiw]

almost there

[davidiw]

why remove this?

[0xjunha]

This first looks into APTOS_INDEXER_URL, but in the case user is running localnet without the --with-indexer-api (and without APTOS_INDEXER_URL) this default value will give us the devnet indexer url, when we're running local tests without an indexer.

Test cases using indexer like test_transfer_coin fails if we keep this default value.

[davidiw]

why this refactor?

[0xjunha]

If we import common.py here,

os.environ["APTOS_FAUCET_URL"] = "http://127.0.0.1:8081"
os.environ["APTOS_NODE_URL"] = "http://127.0.0.1:8080/v1"

of setUpClass method in integration_test.py will not be applied in each test case.

So make integration_test is basically running tests on devnet right now.

[davidiw]

Hmm... because it is setUpClass, let's move these to setUp then. Good call!

[davidiw]

applying 2), we should keep the default value of INDEXER_URL of common.py removed since with the default value it will look into the devnet indexer, failing the integration test.

but how does one even test common otherwise?

[davidiw]

so what you're saying is that this doesn't actually set the environment variable and it is overwritten each time in common.py?

That doesn't make a lot of sense according to the docs.

The way it should work is that we set this once during the class call to "none", each common.py should see that it is set to "none" and leave it alone, during loading of the indexer client we should see it is "none" and not touch it.