feat: transaction with CONTRACT_NEGATIVE_VALUE breaks some routes
#3387
Conversation
Signed-off-by: nikolay <[email protected]>
Test Results 18 files - 4 236 suites - 35 33m 0s ⏱️ - 32m 43s Results for commit 24e6f30. ± Comparison against base commit 373f1dc. This pull request removes 4 and adds 9 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Adding an e2e test is not a straightforward process because that kind of revert can't be reproduced with an I have no problem adding that test if you insist 🚀 🙏. |
Ah I see. Just curious how would you reproduice the problem? |
Signed-off-by: nikolay <[email protected]>
Sending a negative amount via ContractCallTransaction will be enough while it accepts int64 (not uint64). Here is the link to the doc https://docs.hedera.com/hedera/sdks-and-apis/hedera-api/readme-1-1/contractcall#contractcalltransactionbody. |
Ah I see. So if it's feasible and do not take crazy amount of time, I would encourage we have an e2e just to make sure no regression later. Thanks much Nikki! |
Signed-off-by: nikolay <[email protected]>
At the least we could mock the CN response that would cause this and have an integration test that ensures the relay responds as expected. |
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
| try { | ||
| // try to parse it, if the json is valid, numbers within it will be converted | ||
| // this case will happen on almost every GET mirror node call | ||
| return JSONBigInt.parse(data); | ||
| } catch (e) { | ||
| // in some unit tests, the mocked returned json is not property formatted | ||
| // so we have to preprocess it here with JSON.stringify() | ||
| return JSONBigInt.parse(JSON.stringify(data)); | ||
| } |
There was a problem hiding this comment.
Hmmm then should we parse the stringified data at the begining?
There was a problem hiding this comment.
I don't think so, if the passed data can be parsed with no exception that means it's in the right format. We need to stringify it only if the returned data is not already stringified e.g. here https://github.com/hashgraph/hedera-json-rpc-relay/blob/main/packages/relay/tests/lib/mirrorNodeClient.spec.ts#L277 and that happens only in tests (mirror node HTTP requests always return the data stringified).
Another approach is to refactor all tests where non-stringified mocked data is returned. E.g. here https://github.com/hashgraph/hedera-json-rpc-relay/blob/main/packages/relay/tests/lib/mirrorNodeClient.spec.ts#L275
E.g. this:
mock.onGet(`accounts/${alias}${noTransactions}`).reply(200, {
transactions: [
{
nonce: 3,
},
],
links: {
next: null,
},
});should be refactored to:
mock.onGet(`accounts/${alias}${noTransactions}`).reply(200, JSON.stringify({
transactions: [
{
nonce: 3,
},
],
links: {
next: null,
},
}));then we can completely get rid of:
// if the data is not valid, just return it to stick to the current behaviour
if (data) {
try {
// try to parse it, if the json is valid, numbers within it will be converted
// this case will happen on almost every GET mirror node call
return JSONBigInt.parse(data);
} catch (e) {
// in some unit tests, the mocked returned json is not property formatted
// so we have to preprocess it here with JSON.stringify()
return JSONBigInt.parse(JSON.stringify(data));
}
}
return data;and will become just to:
// if the data is not valid, just return it to stick to the current behaviour
if (data) {
return JSONBigInt.parse(data);
}
return data;Does it make sense now? I can create a follow-up issue/PR removing the try/catch and fixing all tests or I can do it in this one but it will become pretty big. 401 tests that need to be fixed if we get rid of try/catch.
822 passing (21s)
401 failing
There was a problem hiding this comment.
@quiet-node just created the follow-up PR #3435.
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3387 +/- ##
==========================================
+ Coverage 84.18% 85.41% +1.23%
==========================================
Files 69 69
Lines 4711 4753 +42
Branches 1048 1059 +11
==========================================
+ Hits 3966 4060 +94
+ Misses 428 396 -32
+ Partials 317 297 -20
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Description:
It seems like
eth_getBlockByNumberfails because one of the transactions in the block was a CONTRACT_NEGATIVE_VALUE failure. The expectation is for the block to be successfully returned.Steps to reproduce:
eth_getBlockByNumberon block 73298898The problematic transaction is here
Related issue(s):
Fixes #3367
Notes for reviewer:
Checklist