Skip to content

Basics of release stake doc#128

Open
ismellike wants to merge 9 commits into
mainfrom
release-stake
Open

Basics of release stake doc#128
ismellike wants to merge 9 commits into
mainfrom
release-stake

Conversation

@ismellike
Copy link
Copy Markdown
Collaborator

@ismellike ismellike commented Dec 12, 2024

closes #125

TODO:

Copy link
Copy Markdown
Collaborator Author

@ismellike ismellike left a comment

Choose a reason for hiding this comment

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

Adding comments to questions section



## Questions
- when it comes to validating the request by the `provider`, should the `provider` always trust the `consumer's` request (`consumer` is always whitelisted) or the `provider` should do additional validation - eg, check that the request doesn't exceed what's already staked?
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think it's fine to trust the consumer's request. The simple things like "exceed what's staked" should be caught by the checked math operations, and the provider doesn't really have any context of how the release_stake amount was calculated anyway.


## Questions
- when it comes to validating the request by the `provider`, should the `provider` always trust the `consumer's` request (`consumer` is always whitelisted) or the `provider` should do additional validation - eg, check that the request doesn't exceed what's already staked?
- do we allow for partial `release_stake` - eg, a consumer would like to free 1/3 of the staked tokens
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it will be up to the provider contract to handle its own stake logic correctly.

## Questions
- when it comes to validating the request by the `provider`, should the `provider` always trust the `consumer's` request (`consumer` is always whitelisted) or the `provider` should do additional validation - eg, check that the request doesn't exceed what's already staked?
- do we allow for partial `release_stake` - eg, a consumer would like to free 1/3 of the staked tokens
- for `token_weighting` contract: what happens when the token-weighting contract receieve a msg to release stake when it's currently calculating new weight? Basically both requests in the same block
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Nice thinking. The only case we need to avoid here is where we might call may_load_at_height at the current height as opposed to the correct may_load. This should already be avoided for the most part, but it's good to keep in mind.

- when it comes to validating the request by the `provider`, should the `provider` always trust the `consumer's` request (`consumer` is always whitelisted) or the `provider` should do additional validation - eg, check that the request doesn't exceed what's already staked?
- do we allow for partial `release_stake` - eg, a consumer would like to free 1/3 of the staked tokens
- for `token_weighting` contract: what happens when the token-weighting contract receieve a msg to release stake when it's currently calculating new weight? Basically both requests in the same block
- for `fan-out` contract: what if we change the denom and request `release_stake` on the old name? It will fail ofc, but I guess we have to call the old denom name.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we support renaming a denom after it has been defined.

@ismellike ismellike marked this pull request as ready for review December 26, 2024 18:56
Copy link
Copy Markdown
Member

@ueco-jb ueco-jb left a comment

Choose a reason for hiding this comment

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

For the basics the doc is as good as it gets I think. I wanted to add to the doc info about the whole stake release flow (so after release_stake user still needs to collect it for example), but in the end this doc should focus on the release_stake message alone.

There are more caveats but I think it will be easier to discuss them in the actual PRs.
Fan Out and Splitter are similar and I think that simply keeping the state of the pending unbondings should be enough.
I was thinking about Operators contract and if we won't allow for more then one unbond at the time per staker (whether it's the unstake or the unset operator messages) we should not have any issues.

What worries me the most is the Delegations implementation, but I would say that might require a separate doc on its own.

Comment on lines +66 to +69
#### 5. Operators
- Maps the **operator address** to the original staker address.
- Updates stake accounting and ensures the message flows through correctly.
- Maintains unbonding claims until the staker calls `ReleaseUnbonded`.
Copy link
Copy Markdown
Member

@ueco-jb ueco-jb Jan 2, 2025

Choose a reason for hiding this comment

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

  • Updates stake accounting and ensures the message flows through correctly.

I was thinking about some specific use cases of unset_operator - that might trigger several unstake events down the line. New operator is set, so the staking new funds (and unstaking those) should be allowed as well.
Now imagine the case with the operators:

  • operator1, staked 1000 token
  • set to operator2, 1000 tokens are locked
  • in the meantime, stake more tokens now to the operator2
  • at this point, should another unset be allowed?

I think this case would be mitigated if we would not allow for more then one unbonding per staker. If you already have an active one, then you can still stake or unstake but that's it.

I know this is a rather general doc, but wanted to share some thoughts.

Comment thread docs/architecture/RELEASE_STAKE.md Outdated
pub struct UnbondingClaim {
pub total_unbonding: Uint128, // Amount being unbonded in this claim
pub released_amount: Uint128, // Amount processed/released so far
pub start_time: u64, // When unbonding started
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would replace that with end_time, since we already know the unbonding time from Collateral plus the start time obviously, why not save the full information.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes good point

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Document how to implement release stake

3 participants