Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return evm_address in eth_getTransactionByHash and eth_getTransactionReceipt #1726

Merged
merged 7 commits into from
Sep 13, 2023

Conversation

Ivo-Yankov
Copy link
Collaborator

@Ivo-Yankov Ivo-Yankov commented Sep 7, 2023

Description:
Changes the to and from addresses returned by eth_getTransactionByHash and eth_getTransactionReceipt to always be evm_addresses instead of long-zero whenever such an address exists. In all other cases a long-zero address is returned.

Related issue(s):

Fixes #1710

Notes for reviewer:

Checklist

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

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
# Conflicts:
#	packages/relay/src/lib/clients/mirrorNodeClient.ts
#	packages/relay/src/lib/eth.ts
#	packages/server/tests/acceptance/rpc_batch1.spec.ts
#	packages/server/tests/helpers/assertions.ts
Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2023

Codecov Report

Patch coverage: 57.14% and project coverage change: +0.08% 🎉

Comparison is base (f28b721) 76.65% compared to head (7c0ea8c) 76.74%.

❗ Current head 7c0ea8c differs from pull request most recent head 0cdf793. Consider uploading reports for the commit 0cdf793 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1726      +/-   ##
==========================================
+ Coverage   76.65%   76.74%   +0.08%     
==========================================
  Files          39       39              
  Lines        2913     2915       +2     
  Branches      584      588       +4     
==========================================
+ Hits         2233     2237       +4     
+ Misses        499      496       -3     
- Partials      181      182       +1     
Files Changed Coverage Δ
packages/relay/src/lib/clients/cache/redisCache.ts 55.88% <0.00%> (ø)
packages/relay/src/lib/eth.ts 85.36% <58.33%> (-0.24%) ⬇️
packages/relay/src/lib/clients/mirrorNodeClient.ts 89.02% <100.00%> (+0.99%) ⬆️

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
@Ivo-Yankov Ivo-Yankov linked an issue Sep 8, 2023 that may be closed by this pull request
@Ivo-Yankov Ivo-Yankov marked this pull request as ready for review September 8, 2023 07:19
@Ivo-Yankov Ivo-Yankov self-assigned this Sep 8, 2023
@Ivo-Yankov Ivo-Yankov added bug Something isn't working limechain labels Sep 8, 2023
@Ivo-Yankov Ivo-Yankov marked this pull request as draft September 8, 2023 07:20
Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
@Ivo-Yankov Ivo-Yankov marked this pull request as ready for review September 8, 2023 11:13
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good.
One consideration based on HIP 729

@@ -1802,8 +1811,10 @@ export class EthImpl implements Eth {
const receipt: any = {
blockHash: toHash32(receiptResponse.block_hash),
blockNumber: numberTo0x(receiptResponse.block_number),
from: receiptResponse.from,
to: receiptResponse.to,
from: receiptResponse.from
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could move the undefined/null check into the resolveEvmAddress()

Assertions.evmAddress(txByHash.from);
Assertions.longZeroAddress(txByHash.to);
Assertions.evmAddress(receipt.from);
Assertions.longZeroAddress(receipt.to);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: post HIP 729 shouldn't all new contract have a valid evm addresses?
I thought we were trying to return evm address values all the time when applicable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I've noticed, contracts that have been deployed via the SDK (using fileCreateTransaction still show a long-zero address in the mirror node instead of a proper evm address. Here are a couple of the latest contracts on testnet:

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very strange.
Can you open an issue on the services side so we can track this nuance.

AlfredoG87
AlfredoG87 previously approved these changes Sep 11, 2023
Copy link
Collaborator

@AlfredoG87 AlfredoG87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.7% 1.7% Duplication

@Nana-EC Nana-EC added this to the 0.32.0 milestone Sep 13, 2023
@Nana-EC Nana-EC added the P1 label Sep 13, 2023
@Ivo-Yankov Ivo-Yankov merged commit c7f0257 into main Sep 13, 2023
@Ivo-Yankov Ivo-Yankov deleted the 1710-transaction-receipt-fixes branch September 13, 2023 09:48
ebadiere pushed a commit that referenced this pull request Sep 18, 2023
…Receipt (#1726)

* wip: fix receipt format

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

* feat: return evm format of from and to address

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

* fix: handle missing to

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

* nit: fix code smells

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

* test: add more test cases

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

* chore: code optimization

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

---------

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
Signed-off-by: ebadiere <ebadiere@gmail.com>
ebadiere pushed a commit that referenced this pull request Sep 20, 2023
…Receipt (#1726)

* wip: fix receipt format

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

* feat: return evm format of from and to address

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

* fix: handle missing to

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

* nit: fix code smells

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

* test: add more test cases

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

* chore: code optimization

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

---------

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
Signed-off-by: ebadiere <ebadiere@gmail.com>
mshakeg pushed a commit to mshakeg/hedera-json-rpc-relay that referenced this pull request Oct 18, 2023
…Receipt (hashgraph#1726)

* wip: fix receipt format

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

* feat: return evm format of from and to address

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

* fix: handle missing to

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

* nit: fix code smells

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

* test: add more test cases

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

* chore: code optimization

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>

---------

Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
Signed-off-by: Mo Shaikjee <shaikjeemohammed@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working limechain P1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong transaction receipt generated for a transfer transaction
4 participants