feat: transaction with CONTRACT_NEGATIVE_VALUE breaks some routes

[natanasow]

Description:

It seems like eth_getBlockByNumber fails 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:

1. call eth_getBlockByNumber on block 73298898
2. see "Error invoking RPC: Invalid value - cannot pass negative number" response

The problematic transaction is here

Related issue(s):

Fixes #3367

Notes for reviewer:

Checklist

• Documented (Code comments, README, etc.)
• Tested (unit, integration, etc.)

[github-actions]

Test Results

 18 files   -   4  236 suites   - 35   33m 0s ⏱️ - 32m 43s
614 tests +  5  610 ✅ + 18  4 💤 +1  0 ❌  - 14 
630 runs   - 216  626 ✅  - 198  4 💤 ±0  0 ❌  - 18 

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.

"after all" hook in "RPC Server Acceptance Tests" ‑ RPC Server Acceptance Tests "after all" hook in "RPC Server Acceptance Tests"
"before all" hook for "should execute "eth_getCode" for hts token" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests eth_getCode "before all" hook for "should execute "eth_getCode" for hts token"
"before all" hook in "@api-batch-2 RPC Server Acceptance Tests" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests "before all" hook in "@api-batch-2 RPC Server Acceptance Tests"
"before each" hook for "from/to Addresses when transferring HTS tokens to the tokenAddress are in evm and long-zero format" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests "before each" hook for "from/to Addresses when transferring HTS tokens to the tokenAddress are in evm and long-zero format"

@release should execute "eth_getCode" for contract evm_address ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests eth_getCode @release should execute "eth_getCode" for contract evm_address
@release should execute "eth_getCode" for contract with id converted to evm_address ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests eth_getCode @release should execute "eth_getCode" for contract with id converted to evm_address
should execute "eth_getBlockByNumber", hydrated transactions = true for a block that contains a call with CONTRACT_NEGATIVE_VALUE status ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-1 RPC Server Acceptance Tests RPC Server Acceptance Tests Block related RPC calls should execute "eth_getBlockByNumber", hydrated transactions = true for a block that contains a call with CONTRACT_NEGATIVE_VALUE status
should execute "eth_getCode" for hts token ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests eth_getCode should execute "eth_getCode" for hts token
should not return contract bytecode after sefldestruct ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests eth_getCode should not return contract bytecode after sefldestruct
should return 0x0 for account alias on eth_getCode ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests eth_getCode should return 0x0 for account alias on eth_getCode
should return 0x0 for account evm_address on eth_getCode ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests eth_getCode should return 0x0 for account evm_address on eth_getCode
should return 0x0 for non-existing contract on eth_getCode ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests eth_getCode should return 0x0 for non-existing contract on eth_getCode
should return empty logs if `toBlock` is not found ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-1 RPC Server Acceptance Tests RPC Server Acceptance Tests eth_getLogs should return empty logs if `toBlock` is not found

♻️ This comment has been updated with latest results.

[quiet-node]

Great work overall. Blocking as one warning needed to be addressed.

Also, is it feasible to add an e2e test to avoid regression?

[natanasow]

Great work overall. Blocking as one warning needed to be addressed.

Also, is it feasible to add an e2e test to avoid regression?

Adding an e2e test is not a straightforward process because that kind of revert can't be reproduced with an EthereumTransaction. In order to re-create it we have to execute ContractCall SDK call which is out of the relay scope IMO.

I have no problem adding that test if you insist 🚀 🙏.

[quiet-node]

Great work overall. Blocking as one warning needed to be addressed.
Also, is it feasible to add an e2e test to avoid regression?

Adding an e2e test is not a straightforward process because that kind of revert can't be reproduced with an EthereumTransaction. In order to re-create it we have to execute ContractCall SDK call which is out of the relay scope IMO.

I have no problem adding that test if you insist 🚀 🙏.

Ah I see. Just curious how would you reproduice the problem?

[natanasow]

Great work overall. Blocking as one warning needed to be addressed.
Also, is it feasible to add an e2e test to avoid regression?

Adding an e2e test is not a straightforward process because that kind of revert can't be reproduced with an EthereumTransaction. In order to re-create it we have to execute ContractCall SDK call which is out of the relay scope IMO.
I have no problem adding that test if you insist 🚀 🙏.

Ah I see. Just curious how would you reproduice the problem?

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.

[quiet-node]

Great work overall. Blocking as one warning needed to be addressed.
Also, is it feasible to add an e2e test to avoid regression?

Adding an e2e test is not a straightforward process because that kind of revert can't be reproduced with an EthereumTransaction. In order to re-create it we have to execute ContractCall SDK call which is out of the relay scope IMO.
I have no problem adding that test if you insist 🚀 🙏.

Ah I see. Just curious how would you reproduice the problem?

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!

[Nana-EC]

Great work overall. Blocking as one warning needed to be addressed.

Also, is it feasible to add an e2e test to avoid regression?

Adding an e2e test is not a straightforward process because that kind of revert can't be reproduced with an EthereumTransaction. In order to re-create it we have to execute ContractCall SDK call which is out of the relay scope IMO.

I have no problem adding that test if you insist 🚀 🙏.

Ah I see. Just curious how would you reproduice the problem?

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!

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.

[quiet-node]

Great work overall! Left some comments and suggestions

[sonarqubecloud]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[quiet-node]

LGTM 1 nit

[codecov]

Codecov Report

Attention: Patch coverage is 84.21053% with 3 lines in your changes missing coverage. Please review.

Project coverage is 85.41%. Comparing base (373f1dc) to head (24e6f30).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/relay/src/formatters.ts	75.00%	2 Missing and 1 partial ⚠️

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     

Flag	Coverage Δ
config-service	98.14% <ø> (ø)
relay	79.30% <100.00%> (+0.08%)	⬆️
server	83.30% <ø> (ø)
ws-server	36.66% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
packages/relay/src/lib/clients/mirrorNodeClient.ts	88.48% <100.00%> (+0.16%)	⬆️
packages/relay/src/formatters.ts	69.56% <75.00%> (+5.51%)	⬆️

... and 7 files with indirect coverage changes