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

fix: Tier module fixes and improvements (part 4) #13

Merged
merged 13 commits into from
Feb 12, 2025

Conversation

iverc
Copy link
Contributor

@iverc iverc commented Jan 20, 2025

Description

This PR aims to add a number of fixes and additional improvements on top of work done in #10

Tasks

  • Add UnlockingLockup, refactor Lockup, GenesisState, and query messages.
  • Rename unbondTime to completionTime to match native staking module.
  • Add creation_height to MsgUnlockResponse.
  • Store lockups / unlocking lockups separately in GenesisState.
  • Fix timestamp and calculation issues in calculateProratedCredit.
  • Improve logic related to storing/retrieving lockups and unlocking lockups.
  • Handle completionTime/unlockTime edge case in CompleteUnlocking.
  • Refactor proratedCredit, add TestCalculateProratedCredit and TestProratedCredit.
  • Add multiple test suites, update/fix a bunch of tests.

@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 87.61062% with 28 lines in your changes missing coverage. Please review.

Project coverage is 29.27%. Comparing base (24c0f5c) to head (810254f).

Files with missing lines Patch % Lines
x/tier/keeper/grpc_query.go 65.51% 6 Missing and 4 partials ⚠️
x/tier/keeper/keeper.go 86.76% 6 Missing and 3 partials ⚠️
x/tier/keeper/lockup.go 90.36% 7 Missing and 1 partial ⚠️
x/tier/types/keys.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev      #13      +/-   ##
==========================================
+ Coverage   28.53%   29.27%   +0.74%     
==========================================
  Files         148      148              
  Lines        6225     6233       +8     
==========================================
+ Hits         1776     1825      +49     
+ Misses       4336     4298      -38     
+ Partials      113      110       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@iverc iverc changed the title fix: Tier module fixes and improvements (part 3) fix: Tier module fixes and improvements (part 4) Jan 20, 2025
@iverc iverc self-assigned this Jan 20, 2025
@iverc iverc requested a review from jsimnz January 20, 2025 17:29
@iverc iverc requested a review from Lodek February 5, 2025 20:52
Copy link
Member

@Lodek Lodek left a comment

Choose a reason for hiding this comment

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

LGTM, I don't fully understand the credit calculations atm but overall the code looks good and the changes make sense after your code walkthrough. Good job Ivan, will review the remaining PRs tomorrow.

Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

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

Really great work here. Like the proper separation between Lockup and UnlockingLockup state (albeit the names are a little confusing, understandably).

Have a few questions, but nothing that is a blocker, and one suggestion that is up to you.

Comment on lines +66 to +68
if sinceCurrentEpoch < epochDurationMs {
credit = credit.MulRaw(sinceCurrentEpoch).QuoRaw(epochDurationMs)
}
Copy link
Member

Choose a reason for hiding this comment

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

praise: nice catch

Copy link
Member

Choose a reason for hiding this comment

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

question/future refactor.

We might want to be more aware of what functions are public/private on the Keeper, or more accurately what functions need to be public.

Reading through some of these Add/Subtract methods makes me feel like they are mostly necessary for the internal functionality to the keeper. Compared to the possibility of another dev using these unnecessarily or accidentally from a different package.

Just a thought. Dont need to be updated in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, we should be more accurate on what needs to be public. Will make these changes in the upcoming PR. Thank you

Comment on lines -92 to -93
// LockupKeyToAddressesAtHeight retreives delAddr, valAddr, and creationHeight from provided unlocking Lockup key.
func LockupKeyToAddressesAtHeight(key []byte) (sdk.AccAddress, sdk.ValAddress, int64) {
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Are the LockupKeyToAddressesAtHeight not still handy util functions to have around?

@iverc iverc merged commit 532dfd4 into dev Feb 12, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants