funding: apportion rewards to voters/LPs #5035
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
848a0dc to
756e7f3
Compare
a13b372 to
67043e4
Compare
erwanor
requested changes
Jan 31, 2025
| position::State::Opened => position::State::Opened, | ||
| position::State::Closed => position::State::Closed, | ||
| position::State::Withdrawn { sequence } => position::State::Withdrawn { | ||
| sequence: sequence.saturating_add(1), |
There was a problem hiding this comment.
I think it would be better to remove the defensive guard, in a separate PR.
There are already two levels of defense:
- the value balance mint/burn flow
- the withdraw position implementation
We are not losing any ground by not having the sequential check in the inner update guard, and we don't have to fabricate withdrawal sequence numbers in return, that seems good
There was a problem hiding this comment.
I like extra defensive guards, but the sequential number bit does seem contrived.
crates/core/component/funding/src/component/liquidity_tournament/bank.rs
Show resolved
Hide resolved
crates/core/component/funding/src/component/liquidity_tournament/bank.rs
Outdated
Show resolved
Hide resolved
| // portion > budget, so eat the whole budget. | ||
| None => (0u64.into(), budget), | ||
| }; | ||
| state.set_lqt_reward_issuance_for_epoch(new_budget); |
There was a problem hiding this comment.
I don't feel super good about rewriting the budget every run rather than letting this be a top-level concern in a single simple loop that it is easy to read. As is, we have to walk through a yo-yo of reads and writes to figure out if this works correctly. And it makes an innocuous method in a different component, totally load-bearing of something that is far away from its context (for example, the keying by epoch is ambient)
There was a problem hiding this comment.
I also think there's a bug here, we want:
and
right?
There was a problem hiding this comment.
Yeah that makes sense, let's just refactor the module to take in amounts, and tie in the budget at one level higher
No implementation yet, but this is the shape I think we want.
67043e4 to
3271f07
Compare
conorsch
pushed a commit
that referenced
this pull request
Jan 31, 2025
## Describe your changes This trait is responsible for the actual accounting of moving funds around, to be later consumed by the end epoch handler, which figures out what portion of the rewards needs to go where. The bank will just actually move fractions of its budget in the community pool to the requested locations, atomically. Testing deferred. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > yes indeed
conorsch
pushed a commit
that referenced
this pull request
Feb 4, 2025
## Describe your changes This trait is responsible for the actual accounting of moving funds around, to be later consumed by the end epoch handler, which figures out what portion of the rewards needs to go where. The bank will just actually move fractions of its budget in the community pool to the requested locations, atomically. Testing deferred. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > yes indeed
2 tasks
conorsch
pushed a commit
that referenced
this pull request
Feb 5, 2025
## Describe your changes This trait is responsible for the actual accounting of moving funds around, to be later consumed by the end epoch handler, which figures out what portion of the rewards needs to go where. The bank will just actually move fractions of its budget in the community pool to the requested locations, atomically. Testing deferred. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > yes indeed
conorsch
pushed a commit
that referenced
this pull request
Feb 5, 2025
## Describe your changes This trait is responsible for the actual accounting of moving funds around, to be later consumed by the end epoch handler, which figures out what portion of the rewards needs to go where. The bank will just actually move fractions of its budget in the community pool to the requested locations, atomically. Testing deferred. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > yes indeed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes
This trait is responsible for the actual accounting of moving funds around, to be later consumed by the end epoch handler, which figures out what portion of the rewards needs to go where. The bank will just actually move fractions of its budget in the community pool to the requested locations, atomically.
Testing deferred.
Checklist before requesting a review
I have added guiding text to explain how a reviewer should test these changes.
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: