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

chore: optimize get last completed upgrade #12268

Merged
merged 3 commits into from
Jul 3, 2022
Merged

chore: optimize get last completed upgrade #12268

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4bd5555
feat: improve GetLastCompletedUpgrade
robert-zaremba Jun 15, 2022
73156d1
rename
robert-zaremba Jun 23, 2022
fde59da
use var block
robert-zaremba Jun 24, 2022
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
22 changes: 8 additions & 14 deletions x/upgrade/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,24 +243,18 @@ func (k Keeper) GetLastCompletedUpgrade(ctx sdk.Context) (string, int64) {
iter := sdk.KVStoreReversePrefixIterator(ctx.KVStore(k.storeKey), []byte{types.DoneByte})
defer iter.Close()

var upgrades []upgrade
var latest upgrade
var found bool
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var latest upgrade
var found bool
var (
latest upgrade
found bool
)

Copy link
Collaborator Author

@robert-zaremba robert-zaremba Jun 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

honestly I prefer simple var rather than var block

Copy link
Contributor

@alexanderbez alexanderbez Jun 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Consistency above all else, please group these.

https://github.com/uber-go/guide/blob/master/style.md#group-similar-declarations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't see this as a standard in our repo.
If we want to enforce uber-go style guide, then we should add it to the collaboration doc, and (ideally) apply linter rule.

I will update the block.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is the standard -- hence why I added the comment. Anytime I see these multi-line var statements, I always comment on the PR to group them. Please group them :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I already did that 4h ago ;) fde59da

for ; iter.Valid(); iter.Next() {
name := parseDoneKey(iter.Key())
value := int64(sdk.BigEndianToUint64(iter.Value()))

upgrades = append(upgrades, upgrade{Name: name, BlockHeight: value})
}

// sort upgrades in descending order by block height
sort.SliceStable(upgrades, func(i, j int) bool {
return upgrades[i].BlockHeight > upgrades[j].BlockHeight
})

if len(upgrades) > 0 {
return upgrades[0].Name, upgrades[0].BlockHeight
if !found || value >= latest.BlockHeight {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets add a comment here that were searching for any upgrade that was past the current blockheight.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've renamed value -> upgradeHeight so I think now the code speaks for itself.

found = true
name := parseDoneKey(iter.Key())
latest = upgrade{Name: name, BlockHeight: value}
}
}

return "", 0
return latest.Name, latest.BlockHeight
}

// parseDoneKey - split upgrade name from the done key
Expand Down