Add ACP-176 e2e tests #3749
Add ACP-176 e2e tests #3749
Conversation
| // AUTOMATICALLY GENERATED. DO NOT EDIT! | ||
| // Generated from hashing.sol by compile-contract.sh |
There was a problem hiding this comment.
This file doesn't seem to have ever been checked in to the repo (even going back to when this file was originally added to the kurtosis e2e tests).
There was a problem hiding this comment.
Complicated by the fact that the toolchain dependencies had to be installed manually... Maybe now is the time to install the necessary deps with nix and implement the compilation in golang so that it can be trivially shared with coreth and subnet-evm?
There was a problem hiding this comment.
I didn't actually use a CLI/golang tool to generate this... I Just used remix (browser app)...
I don't think it's worthwhile to work to setup CI to compile solidity code unless we start writing a bunch of non-trivial solidity and issuing it in CI.
Although the Interop team may already do this to check the teleporter contracts
| return true | ||
| }, DefaultTimeout, DefaultPollingInterval, "failed to see transaction acceptance before timeout") | ||
|
|
||
| require.Equal(types.ReceiptStatusSuccessful, receipt.Status) |
There was a problem hiding this comment.
This implied that we would only want to issue successful transactions. The gas consumption in this PR actually expects the transaction to fail.
There was a problem hiding this comment.
Where is that failure expected? In the 3 calls to SendEthTransaction in dynamic_fees.go, two are checking for a successful status and the 3rd (the one in the Eventually loop for the increase) is unmodified from its previous behavior of ignoring the receipt because the status of the transaction was previously being checked by SendEthTransaction.
Did you mean to tolerate failure in the loop for increase (in which case maybe check the receipt and at least log failure)?
There was a problem hiding this comment.
Also suggest updating the docstring for the function to indicate that the client is responsible for checking that the returned receipt indicates success.
There was a problem hiding this comment.
I updated the doc string.
I suspect there may be some confusion as the what the receipt.Status means. The receipt.Status shows whether the transaction was Successful or if it Reverted. Either way - the transaction was included on the chain and executed. It is normal for transactions to Revert (and in this case it is expected).
I also modified the dynamic_fees test to assert that the status shows that the transaction Reverted (just to make this more clear)
| cChainID, err := ethClient.ChainID(tc.DefaultContext()) | ||
| require.NoError(err) | ||
| signer := types.NewEIP155Signer(cChainID) | ||
| signer := types.NewLondonSigner(cChainID) |
There was a problem hiding this comment.
The london signer supports the DynamicFeeTx
| Value: common.Big0, | ||
| Data: compiledContract, | ||
| compiledContract := common.Hex2Bytes(consumeGasCompiledContract) | ||
| tx := types.NewTx(&types.DynamicFeeTx{ |
There was a problem hiding this comment.
DynamicFeeTxs should realistically always be used where possible. They enable transactions to pay the minimum price, rather than the price they specify.
Specifically, previously the LegacyTx always paid gasTip. The DynamicFeeTx will always pay the gasTipCap for a total less than or equal to the gasFeeCap.
This allows this test to specify such a high gasFeeCap without burning a ton of funds unnecessarily.
|
|
||
| contract ConsumeGas { | ||
| function run() public { | ||
| while (true){} |
There was a problem hiding this comment.
I'm partial to hand-writing EVM bytecode to accomplish this goal, but I guess this is fine 😉
maru-ava
dismissed
their stale review
Accidental approval - need to finish review.
Why this should be merged
This PR:
post_fortunae2e CI run.SuggestGasPriceas the price APIs do not seem reliable. We should consider changing how the price APIs work from inspecting prior blocks to just returning the next baseFee.How this works
Just modifying tests.
How this was tested
CI
Need to be documented in RELEASES.md?
No.