Skip to content

Pass identity balance as string - #4697

Merged
mdomasevicius merged 1 commit into
masterfrom
improvements/amounts
Feb 3, 2022
Merged

Pass identity balance as string#4697
mdomasevicius merged 1 commit into
masterfrom
improvements/amounts

Conversation

@mdomasevicius

@mdomasevicius mdomasevicius commented Feb 1, 2022

Copy link
Copy Markdown
Collaborator

Provide a generic Token object for returning ethereum currency

@mdomasevicius
mdomasevicius force-pushed the improvements/amounts branch 2 times, most recently from 6fffe34 to 8fbd60e Compare February 3, 2022 09:33
@mdomasevicius
mdomasevicius marked this pull request as ready for review February 3, 2022 09:35
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Merging #4697 (6fffe34) into master (c0cbfaa) will decrease coverage by 1.02%.
The diff coverage is 100.00%.

❗ Current head 6fffe34 differs from pull request most recent head 8fbd60e. Consider uploading reports for the commit 8fbd60e to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4697      +/-   ##
==========================================
- Coverage   40.48%   39.46%   -1.03%     
==========================================
  Files         340      355      +15     
  Lines       18538    18960     +422     
==========================================
- Hits         7506     7482      -24     
- Misses      10319    10758     +439     
- Partials      713      720       +7     
Impacted Files Coverage Δ
tequilapi/contract/identity.go 0.00% <ø> (ø)
tequilapi/contract/tokens.go 100.00% <100.00%> (ø)
tequilapi/endpoints/identities.go 33.33% <100.00%> (+0.28%) ⬆️
tequilapi/endpoints/sse_handler.go 69.34% <100.00%> (+0.22%) ⬆️
nat/traversal/pinger.go 78.29% <0.00%> (-5.82%) ⬇️
core/policy/oracle.go 86.48% <0.00%> (-5.41%) ⬇️
session/pingpong/job.go 95.12% <0.00%> (-4.88%) ⬇️
requests/dialer_swarm.go 72.18% <0.00%> (-3.76%) ⬇️
p2p/channel.go 66.17% <0.00%> (-2.21%) ⬇️
core/discovery/discovery.go 63.56% <0.00%> (-1.56%) ⬇️
... and 14 more

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 c0cbfaa...8fbd60e. Read the comment docs.

@tadaskay tadaskay left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Noice

return Tokens{
Wei: amount.String(),
Ether: ethers.String(),
Human: ethers.Truncate(HumanPrecision).String(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Imo the human value here is not something backend should be concerned about. It's not like API should ever consume it in order to specify some amount of tokens and if we want to display it nicely, apps should just convert it to some notation they think is correct. 🤷🏻

When it comes to any interaction with the API we should only really accept Wei.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@tadaskay your position?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We already have this in android API and it has helped a lot to avoid cumbersome logic on client side. Because API is an interface, the most important thing is how easy it is to use it from the consumer point of view. I shouldn't need to deal with the monstrosity where 1 token is presented as a number with 18 zeroes. So thanks for opinion, but this PR solves this long lasting issue.

Signed-off-by: Mantas Domaševičius <mantas@mysterium.network>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants