Conversation
alexanderbez
added
T: State Machine Breaking
fedekunze
requested review from
alessio,
alexanderbez,
jackzampolin and
rigelrozanski
as code owners
alexanderbez
left a comment
alexanderbez
left a comment
There was a problem hiding this comment.
Thanks @fedekunze. I think this PR is a great start! I left some general feedback as I believe some of the API changes are unwarranted. In addition, I don't believe we should be passing iterators to iterator methods -- they should create the iterator internally.
Apart from this, it seems logic has been moved into context-aware files which is nice! So thanks for that ⚡️
Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## master #4460 +/- ##
==========================================
+ Coverage 54.56% 54.71% +0.14%
==========================================
Files 250 253 +3
Lines 16081 16112 +31
==========================================
+ Hits 8775 8815 +40
+ Misses 6658 6651 -7
+ Partials 648 646 -2 |
alexanderbez
left a comment
alexanderbez
left a comment
There was a problem hiding this comment.
A few more minor nitpicks, but otherwise LGTM
Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
| store := ctx.KVStore(keeper.storeKey) | ||
| return store.Iterator(PrefixInactiveProposalQueue, sdk.PrefixEndBytes(PrefixInactiveProposalQueueTime(endTime))) | ||
| // IterateVotes iterates over the all the proposals votes and performs a callback function | ||
| func (keeper Keeper) IterateVotes(ctx sdk.Context, proposalID uint64, cb func(vote types.Vote) (stop bool)) { |
There was a problem hiding this comment.
I would change cb name with something that simplify the understanding of his purpose and or describe it in the comment
There was a problem hiding this comment.
cb is the callback function that's described on the comment
There was a problem hiding this comment.
Exactly, I would give to this callback function a name that reminds us what's his purpose
There was a problem hiding this comment.
I feel the name cb is pretty universal. It's used in Javascript/Node.js too
There was a problem hiding this comment.
Yeah I think cb or handler suffices here 👍
There was a problem hiding this comment.
Why not calling all of them done it's another universal one but helps a bit more.
It's not only related to this PR, everywhere we are using fn, handle or cb for this case, and in all cases what the function does is stop the for loop (plus his callbacky stuff)
There was a problem hiding this comment.
Why not calling all of them done it's another universal one but helps a bit more.
It's not only related to this PR, everywhere we are using fn, handle or cb for this case, and in all cases what the function does is stop the for loop (plus his callbacky stuff)
I think that question should be addressed as part of #3386
alexanderbez
left a comment
alexanderbez
left a comment
There was a problem hiding this comment.
utACK pending simulation passing 👍
jackzampolin
left a comment
jackzampolin
left a comment
There was a problem hiding this comment.
Great code clean up. Looking good @fedekunze 😎
4 participants
closes #4439
closes #4437
Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (
docs/)Added a relevant changelog entry:
clog add [section] [stanza] [message]rereviewed
Files changedin the github PR explorerFor Admin Use: