fix: wrong gas estimation due to invalid tx object #2377
Conversation
georgi-l95
requested review from
AlfredoG87,
ebadiere,
lukelee-sl and
a team
as code owners
packages/relay/src/formatters.ts
Outdated
| * If the transaction has an input property with a length greater than zero and does not have a data property, | ||
| * assign the value of transaction.input to transaction.data and remove the input property. | ||
| */ | ||
| if (transaction.input.length > 0 && !transaction.data) { |
There was a problem hiding this comment.
Great. I fyou can add a bit more info. If I recall correctly it's something around the execution apis changes the spec or some EVM clients use one field and others use the other.
I just want people to have context on why it could be either option
There was a problem hiding this comment.
I've udpated the description of the PR. And for more info, before this fix, when we sent this to the mirror, it returns 0x, which is invalid and this is why we return 400k gas. The reason why it returns 0x, is because we initiate gas estimation for contract create, but our tx object does not have a data field.
Signed-off-by: georgi-l95 <[email protected]> optimization Signed-off-by: georgi-l95 <[email protected]> chore: fix typo Signed-off-by: georgi-l95 <[email protected]> test: add unit test Signed-off-by: georgi-l95 <[email protected]> fix logic Signed-off-by: georgi-l95 <[email protected]> delete input if not used Signed-off-by: georgi-l95 <[email protected]>
georgi-l95
force-pushed
the
2375-issue-with-estimategas-api
branch
from
3925c53
to
2519825
Compare
packages/relay/src/formatters.ts
Outdated
| @@ -79,6 +79,21 @@ const formatTransactionId = (transactionId: string): string | null => { | |||
| return `${payer}-${timestamp}`; | |||
| }; | |||
|
|
|||
| /** | |||
| * Formats a transaction object by moving input to data, if data is absent. | |||
There was a problem hiding this comment.
Would you consider adding an explanation for why such a transformation is necessary here? I mean the PR description explains it very well, and I think incorporating that explanation here in the codebase would make it a good reference for this method, and help developers to understand why it's important without looking into the PR to find the description.
Signed-off-by: georgi-l95 <[email protected]>
georgi-l95
dismissed stale reviews from quiet-node and victor-yanev
via
5eeccdb
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
| // Support either data or input. https://ethereum.github.io/execution-apis/api-documentation/ lists input but many EVM tools still use data. | ||
| if (transaction.input && transaction.data === undefined) { | ||
| transaction.data = transaction.input; | ||
| delete transaction.input; |
There was a problem hiding this comment.
correct me if I'm wrong but I think we didn't have a unit test that makes sure the transaction.input's value is transferred to transaction.data, and transaction.input is deleted from the original txObject.
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
|
* fix: wrong gas estimation due to invalid tx object Signed-off-by: georgi-l95 <[email protected]> optimization Signed-off-by: georgi-l95 <[email protected]> chore: fix typo Signed-off-by: georgi-l95 <[email protected]> test: add unit test Signed-off-by: georgi-l95 <[email protected]> fix logic Signed-off-by: georgi-l95 <[email protected]> delete input if not used Signed-off-by: georgi-l95 <[email protected]> * add simple fix Signed-off-by: georgi-l95 <[email protected]> * remove unused import Signed-off-by: georgi-l95 <[email protected]> * revert changes Signed-off-by: georgi-l95 <[email protected]> * fix acceptance tests Signed-off-by: georgi-l95 <[email protected]> * add unit tests Signed-off-by: georgi-l95 <[email protected]> --------- Signed-off-by: georgi-l95 <[email protected]> Signed-off-by: Alfredo Gutierrez <[email protected]>
fix: wrong gas estimation due to invalid tx object (#2377) * fix: wrong gas estimation due to invalid tx object optimization chore: fix typo test: add unit test fix logic delete input if not used * add simple fix * remove unused import * revert changes * fix acceptance tests * add unit tests --------- Signed-off-by: georgi-l95 <[email protected]> Signed-off-by: Alfredo Gutierrez <[email protected]> Co-authored-by: Georgi Lazarov <[email protected]>
* fix: wrong gas estimation due to invalid tx object Signed-off-by: georgi-l95 <[email protected]> optimization Signed-off-by: georgi-l95 <[email protected]> chore: fix typo Signed-off-by: georgi-l95 <[email protected]> test: add unit test Signed-off-by: georgi-l95 <[email protected]> fix logic Signed-off-by: georgi-l95 <[email protected]> delete input if not used Signed-off-by: georgi-l95 <[email protected]> * add simple fix Signed-off-by: georgi-l95 <[email protected]> * remove unused import Signed-off-by: georgi-l95 <[email protected]> * revert changes Signed-off-by: georgi-l95 <[email protected]> * fix acceptance tests Signed-off-by: georgi-l95 <[email protected]> * add unit tests Signed-off-by: georgi-l95 <[email protected]> --------- Signed-off-by: georgi-l95 <[email protected]>
Description:
This PR adds a formater which checks transaction object, if it complies with the mirror node validations. For example
inputanddatafileds are the same, but mirror node acceptsdatafield as the correct one.For more context:
The official schema accepts both as correct -
dataandinputfield. Where some tools or EVM nodes use one and other use the other. We chose in the mirror node to usedatafield as the correct one, however for us to be able to support all tools, we have to modify transaction object, so that it complies with the mirror node. That means that, ifinputfield is passed, butdatais not, we have to copy one value to the other. For optimization purposes, we can rid of theinputproperty or replace it with empty string.Related issue(s):
Fixes #2375
Notes for reviewer:
Checklist