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

feat: Add option to designate Reward Recipient to Lock and Incentives #5281

Merged
merged 18 commits into from
May 29, 2023

Conversation

mattverse
Copy link
Member

Sub-component of #5044

What is the purpose of the change

This PR introduces a new state entry reward_receiver_address to the period lock. This field would be set to the lock owner by default at the time of lock's creation, and only would be changed by a separate message SetLockRewardReceiverAddress.

Distribution at epoch time would be done to the reward receiver address set in state, instead of directly using the lock owner.

By utilizing this field, further integration with contracts or authz would be possible.

Testing and Verifying

According tests has been added.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

Shouldn't we default rewardReceiver to a blank string, and only use this field if the field has an entry? This would save a bit of storage for all these locks. We also wouldn't need to do a state migration for all pre-existing locks.

@czarcas7ic
Copy link
Member

Also it appears e2e is failing at the create balancer pool step with this change

@czarcas7ic czarcas7ic added the V:state/breaking State machine breaking PR label May 24, 2023
@mattverse
Copy link
Member Author

@czarcas7ic good call, will apply fix and reassign you for review

@czarcas7ic
Copy link
Member

Please ping on slack when its R4R, thank you @mattverse

Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

There are three things that need to be addressed prior to merge:

  • Split lock issue
  • Same reward receiver check in order to merge locks
  • Msg server fix and test

x/incentives/keeper/distribute.go Outdated Show resolved Hide resolved
x/lockup/keeper/bench_test.go Show resolved Hide resolved
x/lockup/keeper/lock.go Outdated Show resolved Hide resolved
x/lockup/keeper/lock.go Outdated Show resolved Hide resolved
x/lockup/keeper/lock.go Show resolved Hide resolved
@@ -94,7 +94,7 @@ func MergeLockupsForSimilarDurations(
}
// create a normalized lock that will absorb the locks in the duration window
normalID = k.GetLastLockID(ctx) + 1
normalLock = types.NewPeriodLock(normalID, owner, normalizedDuration, time.Time{}, lock.Coins)
normalLock = types.NewPeriodLock(normalID, owner, lock.RewardReceiverAddress, normalizedDuration, time.Time{}, lock.Coins)
Copy link
Member

Choose a reason for hiding this comment

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

This also feels like unexpected behavior. What if we have 5 locks, all with different rewardReceiverAddresses? This would set the normalized lock to whatever the final lock's rewardRecieverAddress is. Should we require that all locks have the same rewardReceiverAddress in order to merge them?

Copy link
Member Author

@mattverse mattverse May 27, 2023

Choose a reason for hiding this comment

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

IMO we should not be using this method anymore, since this was a method created use for migration in v7, and our codebase has changed alot since then.

however, I do think that your case states a valid point thus added logic to prevent merging locks if the reward address is not the owner as a saftey measure just in case we do come across using this method in 2d6aa30

Copy link
Member

Choose a reason for hiding this comment

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

If this method is unused, can we remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

We still need the method though since we have the old upgrade handler :(

x/lockup/keeper/msg_server.go Outdated Show resolved Hide resolved
x/lockup/types/msgs.go Outdated Show resolved Hide resolved
x/lockup/types/msgs.go Outdated Show resolved Hide resolved
x/superfluid/keeper/migrate_test.go Outdated Show resolved Hide resolved
Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

Nice, looks solid to me!! Can you ping one more person to get a second ACK? Would feel more comfortable since this is core lockup logic.

Comment on lines 545 to 547
s.Require().Equal(newLock.RewardReceiverAddress, "")
} else {
s.Require().Equal(newLock.RewardReceiverAddress, s.TestAccs[1].String())
Copy link
Member

Choose a reason for hiding this comment

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

nit: these are flipped. The expected value should be the first value and actual should be the second value.

x/lockup/keeper/lock.go Outdated Show resolved Hide resolved
x/lockup/keeper/lock_test.go Outdated Show resolved Hide resolved
Comment on lines +70 to +75

// check if the reward receiver is the lock owner.
// if not, we do not normalize the lock.
if lock.RewardReceiverAddress != "" {
continue
}
Copy link
Member

Choose a reason for hiding this comment

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

ref: https://github.com/osmosis-labs/osmosis/pull/5281/files#r1208613288

I don't understand why we use an empty string placeholder when we could use the owner address directly. Please let me know

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.

This LGTM, nice work! Only had minor comments.

Please ping me for one more skim once the comments are addressed. I would like to see the reply to the "empty string placeholder" discussion

@p0mvn p0mvn merged commit 5ecd4b4 into main May 29, 2023
@p0mvn p0mvn deleted the mattverse/designate branch May 29, 2023 16:17
pysel pushed a commit that referenced this pull request Jun 6, 2023
…#5281)

* Add Reward Recepient to lock

* Add changelog

* Store receiver if receiver is lock owner

* Add defense in depth

* Debuggoing..

* Add debug

* More logs

* Fixed

* Update x/incentives/keeper/distribute.go

* Update x/lockup/keeper/lock.go

* Update x/lockup/keeper/lock_test.go

Co-authored-by: Adam Tucker <[email protected]>

* Update x/lockup/types/msgs.go

Co-authored-by: Adam Tucker <[email protected]>

* Update x/lockup/types/msgs.go

Co-authored-by: Adam Tucker <[email protected]>

* Adams review

* Adam's review

* Fix test

* Add romans comment

---------

Co-authored-by: Adam Tucker <[email protected]>
@github-actions github-actions bot mentioned this pull request Feb 1, 2024
@github-actions github-actions bot mentioned this pull request Apr 1, 2024
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.

3 participants