Limit exec report based on gas estimate limits #56
Conversation
winder
force-pushed
the
will/4-gas-estimate-limit
branch
2 times, most recently
from
653a1e3 to
3e98152
Compare
winder
requested review from
makramkd,
asoliman92,
mateusz-sekara,
0xnogo and
rstout
winder
force-pushed
the
will/4-gas-estimate-limit
branch
from
6822ca0 to
e5ee623
Compare
Test Coverage
|
| @@ -0,0 +1,81 @@ | |||
| // Package evm provides an EVM implementation to the gas.EstimateProvider interface. | |||
| // TODO: Move this package into the EVM repo, chainlink-ccip should be chain agnostic. | |||
There was a problem hiding this comment.
Lets have a ticket for this once you merge these changes 🙏
| var data []byte = msg.Data | ||
| dataLength := len(data) | ||
|
|
||
| // TODO: parse max gas from ExtraArgs. |
There was a problem hiding this comment.
To get this to work in the integration tests, for now I think we can duplicate the implementation here: https://github.com/smartcontractkit/chainlink/blob/6ab3eb5b67739ff88d3c4cf8ea125fd8273bc2b1/core/capabilities/ccip/ccipevm/helpers.go#L9-L13
There was a problem hiding this comment.
Ah dang actually this uses the ABI to decode. We definitely don't wanna import that here
There was a problem hiding this comment.
Maybe we can do a super hacky way for now, something like:
extraArgsNoTag := msg.ExtraArgs[4:]
// gas limit is always the first 32 bytes of ExtraArgs for EVM
gasLimitBytes := extraArgsNoTag[:32]
// convert bytes to big
gasLimit := new(big.Int).SetBytes(gasLimitBytes)Maybe can pull this into a helper func and run a few test cases through it
There was a problem hiding this comment.
I was going to open a ticket and punt on that part until this function has access to go-ethereum.
There was a problem hiding this comment.
That's a good point - maybe implement the EVM version of this interface in the ccipevm package in chainlink? like e.g https://github.com/smartcontractkit/chainlink/blob/6ab3eb5b67739ff88d3c4cf8ea125fd8273bc2b1/core/capabilities/ccip/ccipevm/commitcodec.go#L21
There was a problem hiding this comment.
My understanding is that the ExtraArgs param is abi encoded, so it should be implemented somewhere that go-ethereum can be used, so probably not the ccip / chainlink repos.
There was a problem hiding this comment.
go-ethereum can (at least currently) be used in ccip and chainlink
| "github.com/smartcontractkit/chainlink-common/pkg/types/ccipocr3" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
How this gas cost estimations were made? Any risk of drifting after smart contract change or is the precision we are looking for enough even if a gas cost increase by x%?
There was a problem hiding this comment.
Yes, there is risk of drift. The current logic is taken from here, which are only accurate for 1.5. Hopefully v2 is similar.
There was a problem hiding this comment.
It would probably make sense to check it with onchain team when contracts are defined and audited.
Introduce
gas.EstimateProviderinterface for calculating whether estimated gas requirements should prevent an exec report from being created. This PR wires this interface into the plugin via standard factory methods, but hard codes it to theevm.EstimateProviderimplementation. This implementation will be moved to a separate repo in the future.Use
gas.EstimateProvideras part of theverifyReportcheck which is executed dynamically with different message sets to find the largest allowable report.The new factory parameter is initialized in the ccip repo with this PR: smartcontractkit/ccip#1274