-
Notifications
You must be signed in to change notification settings - Fork 138
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
Check oder of iteration #599
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks solid, but I still would like to stress the importance of #454 when introducing so many refactors and behavior changes for iterators in such a short period of time
ctx := sdk.UnwrapSDKContext(goCtx) | ||
props := k.GetAllConsumerAdditionProps(ctx) | ||
props := types.ConsumerAdditionProposals{} | ||
k.IteratePendingConsumerAdditionProps(ctx, func(prop types.ConsumerAdditionProposal) (stop bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reasoning behind using the iterate method instead of GetAllConsumerAdditionProps
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetAllConsumerAdditionProps
was only used here. Just wanted to cleanup the code. We could revert back if you think it's useful to have it. @MSalopek was also saying he prefers to have it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some nitpicks, but those are not that important.
Would you mind clarifying how #596 will be integrated with regards to this PR? There is a fair amount of overlap between this PR and #596, some sort of agreement and scheduling as to when and where both of these commits get merged seems to be required.
@jtremback can say more on that as he's currently working on #596 |
use sdk func instead of binary.BigEndian.
Close in favor of #620 |
Description
Based on #583:
Linked issues
Closes: #537
Type of change
If you've checked more than one of the first three boxes, consider splitting this PR into multiple PRs!
Fix
: Changes and/or adds code behavior, specifically to fix a bugRefactor
: Changes existing code style, naming, structure, etc.Docs
: Adds documentationRegression tests
If
Refactor
, describe the new or existing tests that verify no behavior was changed or added where refactors were introduced.New behavior tests
Several tests were updated.