Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
0xekez opened this issue Apr 5, 2023 · 5 comments · Fixed by #621
Closed
Assignees
Labels
bug Something isn't working

Comments

@0xekez
Copy link

0xekez commented Apr 5, 2023

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
Copy link
Author

0xekez commented Apr 6, 2023

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: #483

@jonathanpberger jonathanpberger added the bug Something isn't working label Apr 10, 2023
@jonathanpberger
Copy link
Contributor

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
Copy link
Contributor

Hey @0xekez,

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

To get you past this problem see:
#496 - This should give you the ability to pass in that specific --gas auto flag into InstantiateContract
#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: #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
Copy link
Author

0xekez commented Apr 21, 2023

@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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants