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.

Sign up for GitHub

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

Jump to bottom

refactor(x/mint): initialize inflation decimals once #2407

Merged
merged 2 commits into from
Sep 14, 2023
Merged

refactor(x/mint): initialize inflation decimals once #2407

merged 2 commits into from
Sep 14, 2023

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Aug 31, 2023 •
edited
Loading

Closes #2501

@rootulp rootulp added the x/mint label Aug 31, 2023
@rootulp rootulp added this to the Post-mainnet milestone Aug 31, 2023
@rootulp rootulp self-assigned this Aug 31, 2023
@celestia-bot celestia-bot requested a review from a team August 31, 2023 21:00
rach-id
rach-id previously approved these changes Sep 12, 2023
View reviewed changes
evan-forbes
evan-forbes previously approved these changes Sep 13, 2023
View reviewed changes
x/mint/types/constants.go Outdated Show resolved Hide resolved
@staheri14
Copy link
Contributor

Addresses finding 005 of binary builders preliminary audit report.

Is there any GH issue for their finding?

staheri14
staheri14 previously approved these changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

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

LGTM! I'm not entirely certain about the reasoning behind this change (as no GH issue is linked), but the previous approach offers improved encapsulation, especially if these variables might need to be generated from a more intricate logic in the future.

@rootulp
Copy link
Collaborator Author

rootulp commented Sep 13, 2023

Is there any GH issue for their finding?

No they didn't create GH issues for their findings. IDK why not. They shared their preliminary report in Slack so I just DM'ed it to you.

@staheri14
Copy link
Contributor

Is there any GH issue for their finding?

No they didn't create GH issues for their findings. IDK why not. They shared their preliminary report in Slack so I just DM'ed it to you.

Thanks for DM! I think if we could provide some context about the motivation of the PR changes in the PR description, it makes PRs more self-contained and easier to review, wdyt?

@celestia-bot celestia-bot requested a review from a team September 13, 2023 17:44
@rootulp
Copy link
Collaborator Author

rootulp commented Sep 13, 2023

Agreed the normal workflow should be PR closes GH issue and the GH issue should contain context to make it easy to review.

IDK why Binary Builders didn't create GH issues for their findings but I intentionally didn't create GH issues b/c AFAIK their report isn't public.

@rootulp rootulp added the backport:v1.x PR will be backported automatically to the v1.x branch upon merging label Sep 14, 2023
@rootulp
Copy link
Collaborator Author

rootulp commented Sep 14, 2023

Binary Builders created GH issues so I updated all the PRs that close Binary Builders audit findings w/ links to the GH issues (including this one).

@rootulp rootulp merged commit 56312f2 into celestiaorg:main Sep 14, 2023
22 checks passed
mergify bot pushed a commit that referenced this pull request Sep 14, 2023
@rootulp @mergify
refactor(x/mint): initialize inflation decimals once (#2407)
841584f 
Closes #2501

(cherry picked from commit 56312f2)
@mergify mergify bot mentioned this pull request Sep 14, 2023
Merged
rootulp added a commit that referenced this pull request Sep 18, 2023
@mergify @rootulp
refactor(x/mint): initialize inflation decimals once (backport #2407) (…
10fb48d 
…#2513)

This is an automatic backport of pull request #2407 done by
[Mergify](https://mergify.com).


---


<details>
<summary>Mergify commands and options</summary>

<br />

More conditions and actions can be found in the
[documentation](https://docs.mergify.com/).

You can also trigger Mergify actions by commenting on this pull request:

- `@Mergifyio refresh` will re-evaluate the rules
- `@Mergifyio rebase` will rebase this PR on its base branch
- `@Mergifyio update` will merge the base branch into this PR
- `@Mergifyio backport <destination>` will backport this PR on
`<destination>` branch

Additionally, on Mergify [dashboard](https://dashboard.mergify.com) you
can:

- look at your merge queues
- generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com
</details>

Co-authored-by: Rootul P <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staheri14 staheri14 staheri14 approved these changes

@evan-forbes evan-forbes evan-forbes approved these changes

@cmwaters cmwaters Awaiting requested review from cmwaters

@rach-id rach-id Awaiting requested review from rach-id

Assignees

@rootulp rootulp

Labels
backport:v1.x PR will be backported automatically to the v1.x branch upon merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mint module: Initialization of Inflation Constants
4 participants
@rootulp @staheri14 @rach-id @evan-forbes