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

V17 upgrade handler #1029

Merged
merged 26 commits into from
Jan 11, 2024
Merged

V17 upgrade handler #1029

merged 26 commits into from
Jan 11, 2024

Conversation

sampocs
Copy link
Collaborator

@sampocs sampocs commented Dec 14, 2023

Closes #1003

Context

Upgrade handler for v17 upgrade

Note to reviewer: Please pay attention to all constants/string literals and make sure there's no typos in the chain IDs, redemption rate adjustments, rate limit thresholds, etc.

Changelog

  • Registered community pool liquid staking ICA addresses and module addresses
  • Deleted all pending slash queries
  • Reset slash query in progress flag
  • Increased the community pool tax
  • Adjusted the redemption rate bounds
  • Updated the rate limit thresholds
  • Added rate limits to osmosis

@sampocs sampocs marked this pull request as draft December 14, 2023 20:30
@sampocs sampocs added the v17 label Dec 19, 2023
@sampocs sampocs marked this pull request as ready for review December 19, 2023 19:23
Copy link
Contributor

@ethan-stride ethan-stride left a comment

Choose a reason for hiding this comment

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

LGTM

app/upgrades/v17/upgrades.go Show resolved Hide resolved
app/upgrades/v17/upgrades.go Show resolved Hide resolved
Copy link
Contributor

@riley-stride riley-stride left a comment

Choose a reason for hiding this comment

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

Overall looking quite good! Some small fixes we need to make, nothing major.

Also waiting to approve as I think we're still missing:
(1) submit ICA to disable tokenize shares
(2) bank send for shade.

Registered community pool liquid staking ICA addresses and module addresses

lgtm

Deleted all pending slash queries

lgtm

Reset slash query in progress flag

lgtm

Increased the community pool tax

LGTM. Checked it's represented as a decimal in distribution params. New value and IncreaseCommunityPoolTax look good.

Adjusted the redemption rate bounds

Outer bounds lgtm. Osmosis buffer lgtm. Only question is why are we setting the inner bounds in the upgrade handler?

Updated the rate limit thresholds

Need some changes to the percentages per PR comments. Otherwise lgtm.

Added rate limits to osmosis

Looks good for Osmosis, but should we RL other DEXs too?

x/ratelimit/keeper/hooks.go Show resolved Hide resolved
app/upgrades/v17/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v17/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v17/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v17/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v17/upgrades.go Show resolved Hide resolved
app/upgrades/v17/upgrades.go Show resolved Hide resolved
app/upgrades/v17/upgrades.go Outdated Show resolved Hide resolved
@riley-stride
Copy link
Contributor

Note we also need to add the PFM store key. Added a TODO list to the description.

Co-authored-by: sampocs <sam.pochyly@gmail.com>
Copy link
Contributor

@asalzmann asalzmann left a comment

Choose a reason for hiding this comment

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

few small comments, otherwise lgtm!

app/upgrades.go Show resolved Hide resolved
app/upgrades/v17/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v17/upgrades.go Show resolved Hide resolved
app/upgrades/v17/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v17/upgrades.go Show resolved Hide resolved
app/upgrades/v17/upgrades.go Show resolved Hide resolved
@riley-stride
Copy link
Contributor

Checking back in on this one, I think the last few tasks (besides addressing PR comments) are:

  1. Add PFM store key
  2. Submit ICA to disable tokenize shares
  3. Bank send for shade

I've seen some of these in other PRs. Would recommend we get them all merged into here so we have a full final upgrade handler before we sign off on it.

Co-authored-by: sampocs <sam.pochyly@gmail.com>
Copy link
Contributor

@riley-stride riley-stride left a comment

Choose a reason for hiding this comment

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

I see the shade prop, the pfm store key and the ica for preventing tokenization on the hub.

Approved!

@sampocs sampocs merged commit e96f3f8 into main Jan 11, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v17 Upgrade Handler
5 participants