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

fix: change unit of transaction value result #3057

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nadezhdapopovaa
Copy link

@nadezhdapopovaa nadezhdapopovaa commented Oct 3, 2024

Description:

Convert value of a transaction from tinybars to weibars for consistency.

Related issue(s):

Fixes #3053

Notes for reviewer:

Checklist

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

@konstantinabl konstantinabl added the bug Something isn't working label Oct 4, 2024
@konstantinabl konstantinabl added this to the 0.58.0 milestone Oct 4, 2024
@nadezhdapopovaa nadezhdapopovaa force-pushed the 3053-change-units-of-the-value-in-eth-gettransactionbyhash-result branch from 652a601 to cadb710 Compare October 4, 2024 12:01
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.55%. Comparing base (f4a39e3) to head (c828f61).
Report is 1 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (f4a39e3) and HEAD (c828f61). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (f4a39e3) HEAD (c828f61)
relay 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #3057       +/-   ##
===========================================
- Coverage   79.52%   43.55%   -35.97%     
===========================================
  Files          60       50       -10     
  Lines        4044     3788      -256     
  Branches      814      787       -27     
===========================================
- Hits         3216     1650     -1566     
- Misses        551     1863     +1312     
+ Partials      277      275        -2     
Flag Coverage Δ
relay ?

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

Files with missing lines Coverage Δ
packages/relay/src/formatters.ts 49.31% <ø> (ø)

... and 43 files with indirect coverage changes

@nadezhdapopovaa nadezhdapopovaa force-pushed the 3053-change-units-of-the-value-in-eth-gettransactionbyhash-result branch from c828f61 to 108c814 Compare October 7, 2024 12:22
Nadezhda Popova added 3 commits October 7, 2024 15:42
Signed-off-by: Nadezhda Popova <nadezhdapopova@Nadezhdas-Work-MacBook-Pro.local>
Signed-off-by: Nadezhda Popova <nadezhdapopova@Nadezhdas-Work-MacBook-Pro.local>
Signed-off-by: Nadezhda Popova <nadezhdapopova@Nadezhdas-Work-MacBook-Pro.local>
@nadezhdapopovaa nadezhdapopovaa force-pushed the 3053-change-units-of-the-value-in-eth-gettransactionbyhash-result branch from 108c814 to e97dbd5 Compare October 7, 2024 12:43
Copy link

sonarcloud bot commented Oct 7, 2024

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, one suggestion

@@ -176,7 +176,7 @@ const formatContractResult = (cr: any) => {
transactionIndex: nullableNumberTo0x(cr.transaction_index),
type: cr.type === null ? '0x0' : nanOrNumberTo0x(cr.type),
v: cr.v === null ? '0x0' : nanOrNumberTo0x(cr.v),
value: nanOrNumberTo0x(cr.amount),
value: nanOrNumberTo0x(cr.amount * constants.TINYBAR_TO_WEIBAR_COEF),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make a util function for reusability, then you can add a test

Copy link
Member

@quiet-node quiet-node left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

Change units of the value in eth_getTransactionByHash result
4 participants