strangelove-ventures / interchaintest

e2e testing framework for the interchain
https://interchaintest-docs.vercel.app
Apache License 2.0
193 stars 123 forks source link

Interchaintest v4 does not use gas adjustment from chain spec when executing transaction #482

Closed 0xekez closed 1 year ago

0xekez commented 1 year ago

To reproduce, check out the zeke/gas-limits branch and run just integrationtest.

Note that the chain config used sets gas price and adjustment values:

ibc.ChainConfig{
    Denom:          "ujuno",
    GasAdjustment:  2.0,
    GasPrices:      "0.00ujuno",
    EncodingConfig: wasm.WasmEncoding(),
}

But that in test output those values are not used. For example, when instantiating a contract the log output contains:

"command": "junod tx wasm instantiate 2 {\"proxy_code_id\":\"3\",\"block_max_gas\":\"100000000\"} --label wasm-contract --no-admin --from default-juno1-1-xqp --gas-prices 0.00ujuno --gas-adjustment 1.3 --keyring-backend test --output json -y --home /var/cosmos-chain/juno1 --node tcp://juno1-1-fn-0-TestOutOfGas:26657 --chain-id juno1-1"

Note specifically: --gas-adjustment 1.3 in the output which differs from the gas adjustment specified in the configuration.

As always, it's totally possible I'm just doing this wrong. Thanks for your time!


another small thing, the contract instantiation running out of gas does not cause the expected error as ExecTx does not error. Instead, the query for listing the instances of a code ID returns an empty list and the error printed to the console is panic: runtime error: index out of range [-1] while indexing into the code ID list which has a length of zero. you can confirm that the error occuring is an out of gas error only by modifying the interchain test code to print the transaction output of ExecTx like so:

    tx, err := tn.ExecTx(ctx, keyName, command...)
    if err != nil {
        return "", err
    }

    txout, _, err := tn.ExecQuery(ctx, "tx", tx)
    if err != nil {
        return "", err
    }
    tn.logger().Warn("Instantiate TX response", zap.String("out", string(txout)))
0xekez commented 1 year ago

The issue here I have discovered is that interchaintest uses the GasAdjustment value set in ChainSpec, and not the one in ChainConfig. This is a little counterintuitive imo. Here's a spec that sets the gas adjustment to 2.0 for example:

    gasAdjustment := 2.0

    factory := interchaintest.NewBuiltinChainFactory(
        zaptest.NewLogger(t),
        []*interchaintest.ChainSpec{
            {
                Name:          "juno",
                ChainName:     "juno1",
                Version:       "latest",
                GasAdjustment: &gasAdjustment,
                ChainConfig: ibc.ChainConfig{
                    Denom:          "ujuno",
                    GasPrices:      "0.00ujuno",
                    EncodingConfig: wasm.WasmEncoding(),
                },
            },
            {
                Name:          "juno",
                ChainName:     "juno2",
                Version:       "latest",
                GasAdjustment: &gasAdjustment,
                ChainConfig: ibc.ChainConfig{
                    Denom:          "ujuno",
                    GasPrices:      "0.00ujuno",
                    EncodingConfig: wasm.WasmEncoding(),
                },
            },
        },
    )

There is a separate-but-related issue that intercahintest does not set --gas auto when executing messages which makes doing anything with smart contracts pretty hard. I've made a PR for this here: https://github.com/strangelove-ventures/interchaintest/pull/483

jonathanpberger commented 1 year ago

We don't want --gas auto because it'll complain w/ two --gas flags.

Rather, we want to follow this approach that @boojamya has in a branch. He'll follow up here.

boojamya commented 1 year ago

Hey @0xekez,

You bring up a lot of good points in this issue.

To get you past this problem see: https://github.com/strangelove-ventures/interchaintest/pull/496 - This should give you the ability to pass in that specific --gas auto flag into InstantiateContract https://github.com/strangelove-ventures/interchaintest/pull/489 - Will ensure that if you pass in --gas-prices or --gas-adjustment, your value will be used and not the value of the chainSpec/chainConfig

Which brings up another good point. It is indeed confusing that GasAdjustment is in both the chainConfig AND chainSpec In order to break this issue down a bit more, I've created a separate issue for this topic: https://github.com/strangelove-ventures/interchaintest/issues/497

Lastly, do you have any insight as to why the query for listing the instances of a code ID returns an empty list (tn.ExecQuery(ctx, "wasm", "list-contract-by-code", codeID)?

0xekez commented 1 year ago

@boojamya: do you have any insight as to why the query for listing the instances of a code ID returns an empty list

for the case in this issue, I think that happens because instantiation errors and no contract is created. for whatever reason, instantiation erroring doesn't return an error here which causes the problem iirc.

boojamya commented 1 year ago

621 ^^ should fix the last remaining issue brought up here around error handling in the InstantiateContract