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: ADR-009: Supply module #782

Merged
merged 37 commits into from
Apr 8, 2022
Merged

feat: ADR-009: Supply module #782

merged 37 commits into from
Apr 8, 2022

Conversation

leobragaz
Copy link
Contributor

@leobragaz leobragaz commented Mar 18, 2022

Description

Closes: #773 .

This PR implements the x/supply module which serves some useful APIs to query both TotalSupply (converted from millionth) and CirculatingSupply.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@github-actions github-actions bot added x/CLI x/profiles Module that allows to create and manage decentralized social profiles labels Mar 18, 2022
…supply-APIs-module

� Conflicts:
�	app/app.go
�	x/profiles/types/query.pb.go
�	x/profiles/types/query.pb.gw.go
@github-actions github-actions bot removed the x/profiles Module that allows to create and manage decentralized social profiles label Mar 18, 2022
@RiccardoM RiccardoM changed the title Supply module implementation feat: ADR-009: Supply module Mar 21, 2022
@github-actions github-actions bot added the kind/adr An issue or PR relating to an architectural decision record label Mar 22, 2022
- fixed tests
- edited how circulating-supply is returned
…supply-APIs-module

� Conflicts:
�	app/app.go
�	docs/architecture/adr-009-supply-module.md
refactored files
@leobragaz leobragaz marked this pull request as ready for review March 23, 2022 17:04
x/supply/keeper/keeper.go Outdated Show resolved Hide resolved
x/supply/keeper/querier.go Outdated Show resolved Hide resolved
x/supply/module.go Outdated Show resolved Hide resolved
x/supply/module.go Outdated Show resolved Hide resolved
x/supply/module.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 25, 2022

Codecov Report

Merging #782 (90f8ec4) into master (b358016) will increase coverage by 0.01%.
The diff coverage is 82.79%.

❗ Current head 90f8ec4 differs from pull request most recent head df47a9f. Consider uploading reports for the commit df47a9f to get more accurate results

@@            Coverage Diff             @@
##           master     #782      +/-   ##
==========================================
+ Coverage   81.39%   81.41%   +0.01%     
==========================================
  Files          78       81       +3     
  Lines        6747     6839      +92     
==========================================
+ Hits         5492     5568      +76     
- Misses       1001     1013      +12     
- Partials      254      258       +4     
Impacted Files Coverage Δ
x/supply/keeper/querier.go 60.00% <60.00%> (ø)
x/supply/keeper/keeper.go 95.12% <95.12%> (ø)
app/app.go 82.47% <100.00%> (+0.23%) ⬆️
x/supply/keeper/grpc_query.go 100.00% <100.00%> (ø)

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 b358016...df47a9f. Read the comment docs.

x/supply/keeper/keeper.go Outdated Show resolved Hide resolved
@leobragaz leobragaz requested a review from dadamu March 28, 2022 14:36
@leobragaz leobragaz requested a review from dadamu March 29, 2022 14:37
Copy link
Contributor

@dadamu dadamu left a comment

Choose a reason for hiding this comment

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

Looks good to me

proto/desmos/supply/v1/query.proto Outdated Show resolved Hide resolved
proto/desmos/supply/v1/query.proto Outdated Show resolved Hide resolved

// TotalSupply queries the total supply of the given denom, converted with the given divider
rpc TotalSupply(QueryTotalSupplyRequest)
returns (QueryTotalSupplyResponse) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of returning a QueryTotalSupplyResponse, is there any way we can simply return a string here instead? So the REST response instead of looking like this

{
  "total_supply": "<amount>"
}

Can simply look like this:

"<amount>"

Maybe something like StringValue from wrappers.proto can work like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, but that makes the Protobuf linter angry...any way to skip to kind of checks on query returns types? @RiccardoM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Schermata 2022-04-05 alle 17 20 38

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The result isn't what expected btw. Should we roll-back to the previous one?

Copy link
Contributor

@RiccardoM RiccardoM Apr 6, 2022

Choose a reason for hiding this comment

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

Considering CoinMarketCap requires the supply value to be returned simply as a value, we need a way to have this as well. From their docs we can read:

API endpoint that displays ONLY 'Total Supply' as a numerical value (e.g. http://chainz.cryptoid.info/grs/api.dws?q=totalcoins)

Probably the best way would be to register a custom REST endpoint. This can be done using the RegisterRESTRoutes module method and registering a custom endpoint there.

Once that's done, we can simply revert to the previous answers for the gRPC endpoints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both done 👍

@leobragaz leobragaz requested a review from RiccardoM April 5, 2022 13:25
leobragaz and others added 10 commits April 6, 2022 09:52
added missing amino codec
added missing querier initialization to module
now returning an integer
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@RiccardoM RiccardoM added the automerge Automatically merge PR once all prerequisites pass label Apr 8, 2022
@mergify mergify bot merged commit 7481644 into master Apr 8, 2022
@mergify mergify bot deleted the leonardo/coingecko-APIs branch April 8, 2022 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge PR once all prerequisites pass kind/adr An issue or PR relating to an architectural decision record x/CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Coingecko module to expose useful APIs directly from nodes
3 participants