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

Unify iteration approach #3386

Closed
rigelrozanski opened this issue Jan 25, 2019 · 9 comments
Closed

Unify iteration approach #3386

rigelrozanski opened this issue Jan 25, 2019 · 9 comments
Labels
C:x/staking good first issue Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.

Comments

@rigelrozanski
Copy link
Contributor

Going to through the staking code it's obvious that the way we choose to iterate across store records is totally inconsistent, sometimes we use a "retrieveAll" functionality which iterates and returns and array of all the records, which is then received and simply looped over. Other times we use an iterator function which takes an "operating function". Other times we a special keeper function which returns and sdk.Iterator which is then utilized in higher level function. There appears to be duplication of Iterators based on fulfilling the sdk.ValidatorSet/ sdk.DelegationSet

I'm not saying there isn't reason to have some variability in our iteration approach, it just seem that things need to be consolidated and re-evaluated as they've been slapped together.

I'm assuming that this also exists in other modules and should also be rectified.

@rigelrozanski rigelrozanski added C:x/staking post-launch Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. labels Jan 25, 2019
@alexanderbez
Copy link
Contributor

I'm a big fan of the functional callback approach.

@cwgoes
Copy link
Contributor

cwgoes commented Jan 28, 2019

I'm a big fan of the functional callback approach.

Likewise, and it's more efficient (far less allocation) when we don't need to collect records in a list.

@jackzampolin
Copy link
Member

Sounds like the actionable here is to unify around the callback approach. Maybe this is something @fedekunze or @sabau could pick up?

@alexanderbez
Copy link
Contributor

alexanderbez commented May 28, 2019

Correct -- callback approach.

/cc @aayushijain23

@cosmos cosmos deleted a comment from jackzampolin May 28, 2019
@fedekunze
Copy link
Collaborator

Agree on the callback approach, it's cleaner and more flexible

@fedekunze fedekunze mentioned this issue Jun 4, 2019
5 tasks
@sabau
Copy link
Contributor

sabau commented Jun 4, 2019

I found now this issue (thanks @fedekunze ) I love the callback approach idea, was just thinking about the mixed naming I've found across the codebase handle, fn, cb
#4460 (comment)

Why not call all those callbacks done as far as I've seen all those iterators use those callbacks at the end with a format similar to:

if done(...params) {
    break
}

@alexanderbez
Copy link
Contributor

I don't think that reads well at all @sabau imho. Using handler seems like a good compromise, no?

@sabau
Copy link
Contributor

sabau commented Jun 4, 2019

Could work too, that's personal taste, I felt natural the phrase if done then break.
I was mainly trying to suggest to uniform it.

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/staking good first issue Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Projects
None yet
Development

No branches or pull requests

6 participants