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[CL]: remove freezeDuration as a parameter #4724

Merged
merged 76 commits into from
Mar 30, 2023

Conversation

czarcas7ic
Copy link
Member

@czarcas7ic czarcas7ic commented Mar 24, 2023

Closes: #4651

What is the purpose of the change

We no longer need the freezeDuration parameter, as we are now utilizing accumulators for all positions, and simply force users to forfeit accumulated rewards if they leave prior to the rewards minimum uptime charging period.

This PR removes the parameter and refactors various tests to achieve this.

Most notably, we no longer determine forfeiting incentives as binary, and rather iterate a position ID over the various rewards accumulators to determine if the position has existed long enough to claim its share of the rewards, or forfeit them due to not being in the position long enough.

Brief Changelog

Testing and Verifying

This PR refactors existing tests

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
  • How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

@github-actions github-actions bot removed the T:CI label Mar 25, 2023
@github-actions github-actions bot removed the C:docs Improvements or additions to documentation label Mar 25, 2023
@czarcas7ic czarcas7ic added C:docs Improvements or additions to documentation V:state/breaking State machine breaking PR labels Mar 25, 2023
@github-actions github-actions bot removed the C:docs Improvements or additions to documentation label Mar 25, 2023
@czarcas7ic czarcas7ic changed the title DNM: Debug refactor[CL]: remove freezeDuration as a parameter Mar 25, 2023
@@ -101,7 +101,7 @@ func (n *NodeConfig) StoreWasmCode(wasmFile, from string) {

func (n *NodeConfig) WithdrawPosition(from, liquidityOut string, positionId uint64) {
n.LogActionF("withdrawing liquidity from position")
cmd := []string{"osmosisd", "tx", "concentratedliquidity", "withdraw-position", fmt.Sprint(positionId), liquidityOut, fmt.Sprintf("--from=%s", from)}
cmd := []string{"osmosisd", "tx", "concentratedliquidity", "withdraw-position", fmt.Sprint(positionId), liquidityOut, fmt.Sprintf("--from=%s", from), "--gas=auto", "--gas-prices=0.1uosmo", "--gas-adjustment=1.3"}
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: Make an issue regarding this gas adjustment that had to be made for this transaction to go through

Copy link
Member Author

Choose a reason for hiding this comment

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

Issue created #4774

@czarcas7ic czarcas7ic marked this pull request as ready for review March 26, 2023 00:28
Comment on lines 48 to 49
"/osmosis/poolmanager/v1beta1/{pool_id}/estimate_out/"
"single_pool_swap_exact_amount_out";
Copy link
Member Author

Choose a reason for hiding this comment

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

Checked this, apparently it happens automatically from the proto linter, weird.

Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

Would like to see the reply to how the position updates are currently handled.

Other than that, ready to approve. Great work!

x/concentrated-liquidity/store.go Outdated Show resolved Hide resolved
x/concentrated-liquidity/msg_server_test.go Outdated Show resolved Hide resolved
x/concentrated-liquidity/lp_test.go Show resolved Hide resolved
x/concentrated-liquidity/incentives.go Outdated Show resolved Hide resolved
x/concentrated-liquidity/position.go Show resolved Hide resolved
x/concentrated-liquidity/incentives_test.go Show resolved Hide resolved
x/concentrated-liquidity/incentives_test.go Show resolved Hide resolved
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

LGTM. Great work.

It will be helpful to update architecture.md with the state and updates in this PR but not-blocking for merge.

@czarcas7ic czarcas7ic merged commit 3e50c8f into main Mar 30, 2023
@czarcas7ic czarcas7ic deleted the adam/remove-freeze-duration branch March 30, 2023 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:CLI C:simulator Edits simulator or simulations C:x/concentrated-liquidity C:x/gamm Changes, features and bugs related to the gamm module. C:x/poolmanager C:x/superfluid V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CL][Key Refactor] Remove freeze duration as parameter
3 participants