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

feat: TotalCoin implementation #2670

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DIGIX666
Copy link
Contributor

@DIGIX666 DIGIX666 commented Aug 8, 2024

Added TotalCoin implementation to take into account the total of a specific coin based on its denomination.

This should solve #2662

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Aug 8, 2024
@DIGIX666 DIGIX666 changed the title TotalCoin implementation added feat: TotalCoin implementation Aug 8, 2024
Copy link

codecov bot commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

Project coverage is 60.09%. Comparing base (bbcb2f6) to head (6e9287d).

Files Patch % Lines
gno.land/pkg/sdk/vm/builtins.go 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2670      +/-   ##
==========================================
- Coverage   60.11%   60.09%   -0.03%     
==========================================
  Files         560      560              
  Lines       74918    74925       +7     
==========================================
- Hits        45039    45027      -12     
- Misses      26504    26519      +15     
- Partials     3375     3379       +4     
Flag Coverage Δ
contribs/gnodev 60.58% <ø> (-0.82%) ⬇️
contribs/gnofaucet 14.46% <ø> (-0.86%) ⬇️
gno.land 64.07% <0.00%> (-0.10%) ⬇️
gnovm 64.13% <ø> (ø)
misc/genstd 80.54% <ø> (ø)
misc/logos 19.88% <ø> (ø)
tm2 62.03% <ø> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DIGIX666
Copy link
Contributor Author

DIGIX666 commented Aug 9, 2024

Hi @leohhhn , when you have a bit of time, could you check my code please

For the moment, I've put my PR in draft form because I don't think my approach is the right one for these reasons:

  • Iteration on all accounts
  • No persistence for the total result of the part. So every time the function is called, everything is recalculated.

I realised this when I saw the PR for @thinhnx-var

@DIGIX666 DIGIX666 marked this pull request as draft August 9, 2024 16:46
@thinhnx-var
Copy link
Contributor

I did not see this PR after I looked at the original issue again. Thats why I pushed my PR @DIGIX666. 🙏
Btw, I see that the things you commented above is true. Because we should consider about the cost of calculating total coins.

@DIGIX666
Copy link
Contributor Author

I did not see this PR after I looked at the original issue again. Thats why I pushed my PR @DIGIX666. 🙏
Btw, I see that the things you commented above is true. Because we should consider about the cost of calculating total coins.

No problem, I should have asked if someone was working on it first ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: No status
Status: Triage
Development

Successfully merging this pull request may close these issues.

2 participants