Add AirnodeRrpV0DryRun #1737
Add AirnodeRrpV0DryRun #1737
Conversation
| // the calls from AirnodeRrpV0DryRun. | ||
| // Instead, we call an address that we know to not contain any | ||
| // bytecode, which means this call will not revert or spend extra gas. | ||
| (callSuccess, callData) = ADDRESS_WITH_NO_BYTECODE.call( // solhint-disable-line avoid-low-level-calls |
There was a problem hiding this comment.
I didn't want to use address(0) since some chains may have a contract at that address for whatever reason
There was a problem hiding this comment.
I was wondering just that 👍🏻
We can also safely assume that airnode and msg.sender are EOAs without bytecode, wouldn't those be an alternative as well?
There was a problem hiding this comment.
I think you can have msg.sender be a contract (if you can get the requester and the Airnode to play along -- and I think doing so would allow one to build trustless sponsor wallets using https://eips.ethereum.org/EIPS/eip-3074 in the future), but I see how airnode is guaranteed to be an EOA. Do you think that we should use that instead?
There was a problem hiding this comment.
would allow one to build trustless sponsor wallets using https://eips.ethereum.org/EIPS/eip-3074 in the future
🤯
Do you think that we should use that instead?
I don't have a strong opinion about it. It is very unlikely that ADDRESS_WITH_NO_BYTECODE will collide with a smart contract address anyways so I think it's fine too.
There was a problem hiding this comment.
I almost favor calling airnode because it would remove a big line of code
| // the calls from AirnodeRrpV0DryRun. | ||
| // Instead, we call an address that we know to not contain any | ||
| // bytecode, which means this call will not revert or spend extra gas. | ||
| (callSuccess, callData) = ADDRESS_WITH_NO_BYTECODE.call( // solhint-disable-line avoid-low-level-calls |
There was a problem hiding this comment.
I was wondering just that 👍🏻
We can also safely assume that airnode and msg.sender are EOAs without bytecode, wouldn't those be an alternative as well?
| // The estimated gas will be ~40,000. This number can slightly change | ||
| // with future forks, and differ greatly between roll-ups and regular | ||
| // chains. | ||
| expect(estimatedGas.toNumber()).to.greaterThan(30000); | ||
| expect(estimatedGas.toNumber()).to.lessThan(50000); |
There was a problem hiding this comment.
👍🏻
Just being curious, what would the gas more or less be when calling estimateGas on AirnodeRrpV0? I wonder if there is any value in comparing the 2 values in another expect() just to be safe.
There was a problem hiding this comment.
I think it's not guaranteed if it would be less or more because
- AirnodeRrpV0DryRun's fulfill() costs less because it has a minimal external call
- AirnodeRrpV0's fulfill() costs less because it deletes
requestIdToFulfillmentParameters[requestId]
So it would depend on how heavy AirnodeRrpV0's fulfill()'s external call will be, which makes this not a very good test
There was a problem hiding this comment.
Yes, I agree. I'm just trying to see if we can find a more future-proof way of testing the estimageGas() response by using a relative comparison instead of hardcoded numbers that may become obsolete on different chains, etc. But I guess we can do that once (and if) this becomes an issue.
There was a problem hiding this comment.
I was mainly testing that it returns some kind of a number without erroring here. I don't think the value is very important as long as it's something completely wrong like -5 or 10000000000000000000.
| /// will be used to execute a fulfillment successfully. Specifically, since | ||
| /// `eth_estimateGas` looks for the lowest gas limit that results in the | ||
| /// transaction not reverting, and AirnodeRrpV0's `fulfill()` does not revert | ||
| /// when its external call reverts (because it runs out of gas), |
There was a problem hiding this comment.
are you saying it will always return the gas estimate for when the airnode emits FailedRequest becase of the way estimateGas short-circuits ?
There was a problem hiding this comment.
Yes. This was originally picked up by e2e tests for #1589 was failing.
Closes #1716
See the contract comments. I'll address deployment separately, potentially along with #1734