Skip to content
New issue

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

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

Already on GitHub? # to your account

Align receipt with execution apis #332

Merged
merged 3 commits into from
Jul 12, 2022
Merged

Align receipt with execution apis #332

merged 3 commits into from
Jul 12, 2022

Conversation

shemnon
Copy link
Contributor

@shemnon shemnon commented Jul 12, 2022

Description:
Move uints back to hex strings

Signed-off-by: Danno Ferrin danno.ferrin@hedera.com

Related issue(s):

Fixes #302

Notes for reviewer:

Checklist

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

Move uints back to hex strings

Signed-off-by: Danno Ferrin <danno.ferrin@hedera.com>
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.
Some suggestions also i think you're missing test update.
Can see my PR that introduced the change so it's easy to see what to revert

const contractAddress = record.contractFunctionResult == undefined ? undefined : "0x" + record.contractFunctionResult.contractId?.toSolidityAddress();

this.transactionHash = txHash;
this.transactionIndex = 0;
this.blockNumber = Number(block.number);
this.transactionIndex = "0x0";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
this.transactionIndex = "0x0";
this.transactionIndex = EthImpl.zeroHex;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted for a direct rollback on this line.

this.contractAddress = contractAddress;
this.logs = [];
this.logsBloom = '';
this.status = record.receipt.status == Status.Success ? 1 : 0;
this.status = record.receipt.status == Status.Success ? "0x1" : "0x0";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also move 0x1 to a static class variable

Suggested change
this.status = record.receipt.status == Status.Success ? "0x1" : "0x0";
this.status = record.receipt.status == Status.Success ? "0x1" : EthImpl.zeroHex;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted for a direct rollback on this line.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha. 0x0 and 0x1 should still be static class constants

Copy link
Collaborator

Choose a reason for hiding this comment

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

NO biggie. We can clean this and other files up later

@@ -20,6 +20,7 @@

// Used for fake implementation of block history
import {Status, TransactionRecord} from "@hashgraph/sdk";
import {EthImpl} from "./eth";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, seems weird to be importing this just for a formatting method.
Eventually (not a fix for now) I think we should move the converter functions out to the formatter.ts class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted for a direct rollback in other parts of code so this isn't needed.

@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2022

Codecov Report

Merging #332 (2dbd914) into main (334345b) will not change coverage.
The diff coverage is 16.66%.

@@           Coverage Diff           @@
##             main     #332   +/-   ##
=======================================
  Coverage   62.44%   62.44%           
=======================================
  Files           9        9           
  Lines         868      868           
  Branches      141      141           
=======================================
  Hits          542      542           
  Misses        288      288           
  Partials       38       38           
Impacted Files Coverage Δ
packages/relay/src/lib/eth.ts 64.25% <ø> (ø)
packages/relay/src/lib/model.ts 82.82% <16.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 334345b...2dbd914. Read the comment docs.

Signed-off-by: Danno Ferrin <danno.ferrin@hedera.com>
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.

LG.
"should execute "eth_getTransactionReceipt" for hash..." acceptance tests need updating with your new logic

this.contractAddress = contractAddress;
this.logs = [];
this.logsBloom = '';
this.status = record.receipt.status == Status.Success ? 1 : 0;
this.status = record.receipt.status == Status.Success ? "0x1" : "0x0";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha. 0x0 and 0x1 should still be static class constants

this.contractAddress = contractAddress;
this.logs = [];
this.logsBloom = '';
this.status = record.receipt.status == Status.Success ? 1 : 0;
this.status = record.receipt.status == Status.Success ? "0x1" : "0x0";
Copy link
Collaborator

Choose a reason for hiding this comment

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

NO biggie. We can clean this and other files up later

Signed-off-by: Danno Ferrin <danno.ferrin@hedera.com>
@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
0.0% 0.0% Duplication

@Nana-EC Nana-EC added bug Something isn't working P2 labels Jul 12, 2022
@Nana-EC Nana-EC added this to the 0.4.0 milestone Jul 12, 2022
@shemnon shemnon merged commit c3a8c74 into main Jul 12, 2022
@shemnon shemnon deleted the 302-uints-are-strings-too branch July 12, 2022 23:35
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working P2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eth_getTransactionReceipt invalid response: hex string instead of uint
3 participants