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: make tickers module keep token exponents too #78

Merged
merged 3 commits into from
Aug 22, 2024

Conversation

hallazzang
Copy link
Contributor

@hallazzang hallazzang commented Aug 11, 2024

Description

Instead of simply keeping denom-ticker mappings, this PR introduces new Asset type which holds the exponent as well. With this change it's possible to calculate dollar values of an asset correctly with assets with different exponents.

Closes: MILK-66

Depends-On: #63


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)

@hallazzang hallazzang marked this pull request as ready for review August 11, 2024 13:17
@hallazzang hallazzang self-assigned this Aug 11, 2024
@RiccardoM
Copy link
Contributor

@hallazzang can you rebase this please? 🙏

@hallazzang hallazzang force-pushed the hallazzang/new-rewards-module branch 2 times, most recently from bd18713 to 69cc147 Compare August 12, 2024 12:34
@hallazzang hallazzang force-pushed the hallazzang/ticker-exponent branch from f0d97cd to c871758 Compare August 12, 2024 12:56
@hallazzang
Copy link
Contributor Author

@hallazzang can you rebase this please? 🙏

Done!

@hallazzang hallazzang force-pushed the hallazzang/ticker-exponent branch from 68ce1cc to 811b64a Compare August 12, 2024 13:44
@RiccardoM
Copy link
Contributor

@hallazzang Can you please resolve the conflicts, and remove all the changes related to the rewards module from this branch (you can see the Changed Files tab for a reference). Thanks 🙏

@hallazzang
Copy link
Contributor Author

remove all the changes related to the rewards module from this branch (you can see the Changed Files tab for a reference)

But these changes to the rewards module have been made because of the changes in this PR. Without updating the rewards module accordingly, build will fail.

@RiccardoM
Copy link
Contributor

But these changes to the rewards module have been made because of the changes in this PR. Without updating the rewards module accordingly, build will fail.

Got it, thanks. Will mark this module as ready to be merged once the rewards module is merged then (will add the dependency into the description).

Base automatically changed from hallazzang/new-rewards-module to main August 20, 2024 15:58
@RiccardoM
Copy link
Contributor

@hallazzang Could you please rebase/merge main into this one?

@hallazzang
Copy link
Contributor Author

hallazzang commented Aug 20, 2024

Closing this PR because I opened a new PR #89. Reopened.

@hallazzang hallazzang closed this Aug 20, 2024
@hallazzang hallazzang reopened this Aug 21, 2024
@hallazzang hallazzang force-pushed the hallazzang/ticker-exponent branch from 33c6722 to 1b3a738 Compare August 21, 2024 15:27
@hallazzang hallazzang force-pushed the hallazzang/ticker-exponent branch from 1b3a738 to a5f520e Compare August 21, 2024 15:30
@RiccardoM RiccardoM merged commit 1c37a2b into main Aug 22, 2024
18 checks passed
@RiccardoM RiccardoM deleted the hallazzang/ticker-exponent branch August 22, 2024 14:06
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.

2 participants