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

refactor: interchainstaking/keeper #330

Merged
merged 10 commits into from
Mar 3, 2023
Merged

refactor: interchainstaking/keeper #330

merged 10 commits into from
Mar 3, 2023

Conversation

aljo242
Copy link

@aljo242 aljo242 commented Mar 3, 2023

1. Summary

interchainstaking is our most complicated module and package in the codebase. This PR attempts to refactor and modularize the keeper package. Maintenance like this can help us with adding features, instrumentation and testing moving forward.

2.Type of change

  • refactor

3. Implementation details

  • move non-keeper methods to the types package
  • modularize function calls more
  • unify keeper function signatures so that they are all pointer receivers
  • pass Zone struct as a pointer to our functions

4. How to test/use

Verify functionality is not modified by this PR.

5. Checklist

  • Does the Readme need to be updated?

6. Limitations (optional)

7. Future Work (optional)

@joe-bowman
Copy link
Contributor

Any idea why no checks have run here, @aljo242 ?

@aljo242
Copy link
Author

aljo242 commented Mar 3, 2023

Any idea why no checks have run here, @aljo242 ?

Was wondering the same thing - no idea

@aljo242
Copy link
Author

aljo242 commented Mar 3, 2023

@joe-bowman running now after I merged main. Weird

@codecov-commenter
Copy link

codecov-commenter commented Mar 3, 2023

Codecov Report

Merging #330 (c7d3473) into main (f067363) will decrease coverage by 0.88%.
The diff coverage is 67.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #330      +/-   ##
==========================================
- Coverage   55.33%   54.46%   -0.88%     
==========================================
  Files         155      157       +2     
  Lines       11649    11689      +40     
==========================================
- Hits         6446     6366      -80     
- Misses       4745     4870     +125     
+ Partials      458      453       -5     
Flag Coverage Δ
unittests 54.46% <67.52%> (-0.88%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
app/app.go 58.79% <0.00%> (ø)
x/interchainstaking/types/keys.go 47.82% <0.00%> (-52.18%) ⬇️
...pationrewards/keeper/rewards_validatorSelection.go 78.57% <ø> (ø)
x/interchainstaking/types/rebalance.go 28.87% <28.87%> (ø)
x/interchainstaking/keeper/receipt.go 25.94% <41.66%> (-0.70%) ⬇️
x/interchainstaking/keeper/ibc_packet_handlers.go 31.83% <58.33%> (+0.46%) ⬆️
x/interchainstaking/keeper/zones.go 44.14% <58.82%> (ø)
x/interchainstaking/keeper/abci.go 42.22% <60.00%> (+2.68%) ⬆️
x/interchainstaking/keeper/proposal_handler.go 25.90% <63.63%> (+0.50%) ⬆️
x/airdrop/keeper/claim_handler.go 59.31% <66.66%> (ø)
... and 17 more

@joe-bowman
Copy link
Contributor

@aljo242 bunch of conflicts now!

Copy link
Contributor

@joe-bowman joe-bowman left a comment

Choose a reason for hiding this comment

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

lgtm

@joe-bowman
Copy link
Contributor

@ajansari95

Copy link
Contributor

@ajansari95 ajansari95 left a comment

Choose a reason for hiding this comment

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

LGTM

@ajansari95 ajansari95 merged commit 607f847 into main Mar 3, 2023
@ajansari95 ajansari95 deleted the chore/clean-ics branch March 3, 2023 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants