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

add total superfluid delegation sum invariant #1018

Closed
wants to merge 9 commits into from

Conversation

antstalepresh
Copy link
Contributor

Closes: #XXX

Description


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM to merge once the API comment is resolved

@ValarDragon ValarDragon mentioned this pull request Mar 7, 2022
10 tasks
@ValarDragon
Copy link
Member

A couple of options:

  • Refresh delegation immediately after a slash. (Make a new hook called AfterValidatorSlashed)
  • Slash in parity with existing SDK code (not preferrable, existing SDK code is the one thats wrong)
  • Make the invariant have an exception if in the same epoch as a slash
  • Check the rounding on slashing. (Slash in SDK rounds once per IA, in our code rounds once every underlying lockup)
    • The answer is still refresh immediately, or make make the invariant have an edge case for same epoch as slash

go.mod Outdated
github.com/CosmWasm/wasmd => github.com/osmosis-labs/wasmd v0.22.0-osmo-v7.2
// Our cosmos-sdk branch is: https://github.com/osmosis-labs/cosmos-sdk v0.45.0x-osmo-v7
github.com/cosmos/cosmos-sdk => github.com/osmosis-labs/cosmos-sdk v0.45.1-0.20220221115656-75545ea8cb2d
github.com/cosmos/cosmos-sdk => github.com/osmosis-labs/cosmos-sdk v0.45.1-0.20220309174635-d81fb5657a15
Copy link
Member

Choose a reason for hiding this comment

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

Can you PR the hook addition to the SDK branch, then we can get this merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ValarDragon
Copy link
Member

Just needs to update the go.mod reference to be merged commit in the SDK branch, and we can get this merged.

@antstalepresh antstalepresh changed the base branch from v7.x to main March 13, 2022 15:32
@antstalepresh antstalepresh changed the base branch from main to v7.x March 13, 2022 15:32
@codecov-commenter
Copy link

Codecov Report

Merging #1018 (815b550) into v7.x (8ce7b0d) will increase coverage by 0.11%.
The diff coverage is 60.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##             v7.x    #1018      +/-   ##
==========================================
+ Coverage   20.59%   20.70%   +0.11%     
==========================================
  Files         193      194       +1     
  Lines       25231    25279      +48     
==========================================
+ Hits         5197     5235      +38     
- Misses      19082    19088       +6     
- Partials      952      956       +4     
Impacted Files Coverage Δ
x/claim/keeper/hooks.go 28.00% <ø> (ø)
x/superfluid/keeper/hooks.go 61.90% <33.33%> (-4.77%) ⬇️
x/superfluid/keeper/invariants.go 54.05% <54.05%> (ø)
x/superfluid/keeper/intermediary_account.go 88.15% <100.00%> (+1.39%) ⬆️
x/superfluid/keeper/stake.go 58.82% <0.00%> (+4.70%) ⬆️
x/superfluid/keeper/keeper.go 100.00% <0.00%> (+6.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ce7b0d...815b550. Read the comment docs.

@antstalepresh
Copy link
Contributor Author

Here's cherry picked version of this PR that is to main branch. And closing this.
#1081

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants