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

fix(x/epochs): avoid invalid epoch duration in simulation #20030

Merged
merged 4 commits into from
Apr 12, 2024
Merged

fix(x/epochs): avoid invalid epoch duration in simulation #20030

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
526f500
fix(x/epochs): avoid invalid epoch duration in simulation
mmsqe Apr 12, 2024
c212974
update doc
mmsqe Apr 12, 2024
cd39b88
Apply suggestions from code review
mmsqe Apr 12, 2024
a2cb4e5
Apply suggestions from code review
mmsqe Apr 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i
* [#19851](https://github.com/cosmos/cosmos-sdk/pull/19851) Fix some places in which we call Remove inside a Walk (x/staking and x/gov).
* (baseapp) [#19970](https://github.com/cosmos/cosmos-sdk/pull/19970) Fix default config values to use no-op mempool as default.
* (x/bank) [#20028](https://github.com/cosmos/cosmos-sdk/pull/20028) Align query with multi denoms for send-enabled.
* (simulation) [#20030](https://github.com/cosmos/cosmos-sdk/pull/20030) Avoid invalid epoch duration in x/epochs simulation.
Copy link
Member

Choose a reason for hiding this comment

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

we don't need a changelog for this.


### API Breaking Changes

Expand Down
2 changes: 1 addition & 1 deletion x/epochs/simulation/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (

// GenCommunityTax randomized CommunityTax
func GenDuration(r *rand.Rand) time.Duration {
return time.Hour * time.Duration(r.Intn(168)) // limit 1 week
return time.Hour * time.Duration(r.Intn(167)+1) // between 1 hour to 1 week
Copy link
Contributor

@coderabbitai coderabbitai bot Apr 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ensure the upper bound of the random duration is correctly set. The current implementation (r.Intn(167)+1) generates a number between 1 and 167, which corresponds to 1 hour to 167 hours (not a full week). Consider adjusting this to r.Intn(168)+1 to include the full range up to 168 hours (1 week).

- return time.Hour * time.Duration(r.Intn(167)+1) // between 1 hour to 1 week
+ return time.Hour * time.Duration(r.Intn(168)+1) // between 1 hour to 1 week
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
return time.Hour * time.Duration(r.Intn(167)+1) // between 1 hour to 1 week
return time.Hour * time.Duration(r.Intn(168)+1) // between 1 hour to 1 week

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we follow the original logic which is 167 hours

Copy link
Contributor

Choose a reason for hiding this comment

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

@mmsqe, the suggestion to adjust the upper bound to 168 hours was aimed at ensuring the duration could fully cover up to a complete week (168 hours). This adjustment was intended to enhance the accuracy of the simulation's time span.

However, if there's a specific reason or context within the simulation framework that makes 167 hours more appropriate, or if aligning exactly to a week introduces any issues, it would be helpful to understand that context. Could you please share if there's a particular reason for preferring the original logic of 167 hours? This will help in making the most informed decision.

}

func RandomizedEpochs(r *rand.Rand) []types.EpochInfo {
Expand Down
Loading