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

Implement Stake Pre-Splitting #444

Merged
merged 14 commits into from
Dec 5, 2024
Merged

Implement Stake Pre-Splitting #444

merged 14 commits into from
Dec 5, 2024

Conversation

JSKitty
Copy link
Member

@JSKitty JSKitty commented Oct 31, 2024

Abstract

This PR adds a simple but very useful improvement to staking efficiency for MPW, by pre-splitting large delegations, we allow each output to confirm and start staking individually without causing one stake to put the entire balance in to a long maturation period.

Take these two examples of how this change could improve the user experience:

Example 1: unusually large 'immature balances' confusing users

If a user delegates 10,000 PIVX (their entire balance), and it hits a stake, their entire balance is going to be considered immature, and thus unusable, which is not ideal - this situation can be prevented by pre-splitting delegated UTXOs.

Example 2: slower stake rewards, shortly after delegating

In the same example of staking 10,000 PIV: if you delegate a single UTXO of 10,000, which hits a stake - then that entire UTXO will not be staking until it re-matures, and the Cold Address splits this automatically with PIVX Core's stakesplitthreshold, this wasted time ends up reducing the rewards you could potentially earn in the short-term.

With pre-splitting in to, say, 20x 500 PIV UTXOs, hitting a stake means only 500 PIV is immature, and the other 9,500 PIV continues staking, which means you could earn more stakes in the same time period that it takes for your initial 10k to re-mature and become stakeable again.


Note

The PR's suggested split target is 500 PIV - the same setting that the PIVX Labs Cold Pool uses for it's stakesplitthreshold.

Mainnet Example

Using this PR, configured at a 5 PIV split threshold, a new large delegation looks like:
https://explorer2.duddino.com/tx/295a547273879d744d5cd34a0cf09ba973842aa53023a0425407f0a9d91b5390


Testing

To test this PR, it's suggested to attempt these user flows, or variations of these:

  • Stake 500 or less PIV: ensure it is a single UTXO (vout) on the explorer.
  • Stake over >500 PIV: ensure the outputs 'split' in to even 500-per-out UTXOs, and the remainder is less than 500 PIV.

If any errors are found, the PR works unexpectedly, or you have viable suggestions to improve the UX or functionality of the PR, let me know!


This was causing change to be computed incorrectly if the actual `valueOut` differed from the user's `value` due to automated differences (i.e: Stake Split remainders)
@JSKitty JSKitty added the Enhancement New feature or request label Oct 31, 2024
@JSKitty JSKitty self-assigned this Oct 31, 2024
Copy link

netlify bot commented Oct 31, 2024

Deploy Preview for cheery-moxie-4f1121 ready!

Name Link
🔨 Latest commit 12816bb
🔍 Latest deploy log https://app.netlify.com/sites/cheery-moxie-4f1121/deploys/675184c0f7f04d00089ca76e
😎 Deploy Preview https://deploy-preview-444--cheery-moxie-4f1121.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@panleone panleone left a comment

Choose a reason for hiding this comment

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

Left a few comments, but overall I like the feature

scripts/wallet.js Outdated Show resolved Hide resolved
scripts/wallet.js Outdated Show resolved Hide resolved
Copy link
Member

@panleone panleone left a comment

Choose a reason for hiding this comment

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

a couple of comments

scripts/wallet.js Outdated Show resolved Hide resolved
tests/unit/wallet/transactions.spec.js Outdated Show resolved Hide resolved
Copy link
Member

@panleone panleone left a comment

Choose a reason for hiding this comment

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

Last few comments about test changes, everything else is good.

tests/unit/wallet/transactions.spec.js Show resolved Hide resolved
tests/unit/wallet/transactions.spec.js Outdated Show resolved Hide resolved
tests/unit/wallet/transactions.spec.js Outdated Show resolved Hide resolved
tests/unit/wallet/transactions.spec.js Outdated Show resolved Hide resolved
Duddino
Duddino previously approved these changes Dec 5, 2024
Copy link
Member

@Duddino Duddino left a comment

Choose a reason for hiding this comment

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

tACK other than panleone's test nit

@Duddino Duddino requested a review from panleone December 5, 2024 10:37
Copy link
Member

@panleone panleone left a comment

Choose a reason for hiding this comment

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

tACK 12816bb

and merging

@panleone panleone merged commit 06eef78 into master Dec 5, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants