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

Allow swapping AutoCompounding shares for ManualReward shares (and the other way around) #273

Merged
merged 13 commits into from
Sep 29, 2023

Conversation

nanocryk
Copy link
Contributor

Currently a delegator needs to undelegate and delegate again to change in which pool their stake is, which takes time as they need to wait for the joining and leaving delays. However there is no security risk in changing pool, so it should be instant instead. This PR adds a new call to go from one pool to another, with the dust from joining the target pool considered leaving stake.

TODO: Benchmark

@github-actions
Copy link
Contributor

github-actions bot commented Sep 22, 2023

Master coverage: 71.90%
Coverage generated "Thu Sep 28 14:04:06 UTC 2023":
https://d3gkbsry1ehhqi.cloudfront.net/tanssi-coverage/pulls/273/index.html
Pull coverage: 72.46%

@girazoki
Copy link
Collaborator

So a couple of things missing on this PR yet, so that we dont forget:

  • integrations tests in rust
  • integrations tests in TS
  • benchmarking

) -> DispatchResultWithPostInfo {
// Converts amount to shares of the correct pool
let old_shares = match (amount, source_pool) {
(SharesOrStake::Shares(s), _) => s,
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this supposed to cover shares from joining or leaving pools? what would happen in this case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah never mind, target pool only contains automcpounding and manual rewards. So its only to cover cases in which the amount is given as shares

Copy link
Collaborator

@girazoki girazoki left a comment

Choose a reason for hiding this comment

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

I'm happy with the way the PR looks, but I think we should add benchmark + integration tests before merging

Copy link
Collaborator

@girazoki girazoki left a comment

Choose a reason for hiding this comment

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

I think if we add a simple typescript test we this PR can get merged

@nanocryk nanocryk merged commit e3847f0 into master Sep 29, 2023
@nanocryk nanocryk deleted the jeremy-staking-pool-swap branch September 29, 2023 10:30
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.

3 participants