[WIP] Staking rewards module #226
Conversation
save progress
## Description - Reviewers should test against local mainnet state. Sebs EC2 instance did not have a port exposed so I went with the mainnet mirror. Which now has the feature reconfig applied. Note that the feature `CALCULATE_TRANSACTION_FEE_FOR_DISTRIBUTION = 96` can only be enabled after Move2 upgrade. Hence why it is not enabled here (it is not available on `movement-migration` framework) - See the script `feature-flag-reconfig.move` for what features are being enabled and disabled. Note that after Move2 upgrade, further features to be enabled which are only available after the upgrade. 1. Compile the feature flag upgrade script ``` movement move compile --package-dir movement-migration/framework-upgrades --bytecode-version 6 ``` 2.Run the script ``` movement move run-script --compiled-script-path movement-migration/framework-upgrades/build/feature-flag-reconfig/bytecode_scripts/main.mv ``` Should receive success output. Mainnet Mirror tx ``` Do you want to submit a transaction for a range of [11200 - 16800] Octas at a gas unit price of 100 Octas? [yes/no] > y Transaction submitted: https://explorer.movementlabs.xyz/txn/0x7a92eff1e1e973e881a046aaebaa72e42a1f6a1daaa3bec45167657c8d8c67ad?network=custom { "Result": { "transaction_hash": "0x7a92eff1e1e973e881a046aaebaa72e42a1f6a1daaa3bec45167657c8d8c67ad", "gas_used": 112, "gas_unit_price": 100, "sender": "000000000000000000000000000000000000000000000000000000000a550c18", "sequence_number": 12966, "success": true, "timestamp_us": 1759411858121293, "version": 28646125, "vm_status": "Executed successfully" } } ```
musitdev
left a comment
musitdev
left a comment
There was a problem hiding this comment.
My main wondering , I don't understand Why you use a new config for stacking and not the StakingRewardsConfig that is used by the staking module?
| admin: &signer, | ||
| target_apr_percentage_x100: u64, // APR in 1/100 %, e.g. 625 => 6.25% | ||
| epochs_per_year: u64, // e.g. 4_380 | ||
| denominator: u64, // typically 1_000_000_000 |
There was a problem hiding this comment.
Rename by rewards_rate_den to be compatible with the specs.
There was a problem hiding this comment.
id say we can adopt Aptos variable names and can change our specs instead.
|
|
||
| // per_epoch_num = ((APR(bp) * den) / 100) / epochs_per_year | ||
| let num_u128 = (((annual_apr_bp as u128) * (denominator as u128)) / 100u128) / (epochs_per_year as u128); | ||
| let per_epoch_num = num_u128 as u64; |
There was a problem hiding this comment.
Rename per_epoch_num with rewards_rate_num
| assert!(!option::is_some(&g.config), error::already_exists(E_ALREADY_INITIALIZED)); | ||
|
|
||
| // Convert APR x100 to basis points: 1% = 100 bp (so x100 == bp). | ||
| let annual_apr_bp = target_apr_percentage_x100; |
There was a problem hiding this comment.
Rename annual_apr_bp to target_apr_percentage to be compatible with the specs.
| cfg.rewards_rate_den = new_rate_den; | ||
|
|
||
| // Keep annual APR in-sync for transparency (approx). | ||
| cfg.annual_apr_bp = recompute_annual_bp_from_epoch_rate(new_rate_num, new_rate_den, cfg.epochs_per_year); |
There was a problem hiding this comment.
Why you keep the annual_apr. We don't need it, and it can be calculated off chain. We need to be sure the recompute_annual_bp_from_epoch_rate is correct and won't create any issues.
It's easier to keep only the data we need to compute the reward.
| emit_reward_rate_updated(&mut g.events, 0, 1, per_epoch_num, denominator, &string::utf8(b"init"), /*epoch_effective*/ 0); | ||
| } | ||
|
|
||
| /// Manual Governance Update (Path 1) |
There was a problem hiding this comment.
I think we need to decide the scenario and integrate it with the staking module.
|
|
||
| // Ensure not already initialized (close the borrow with a block and semicolon). | ||
| { | ||
| let g_ref = borrow_global_mut<Global>(@aptos_framework); |
There was a problem hiding this comment.
Why you borrow_mut a read borrow is enough?
|
|
||
| // Write config | ||
| { | ||
| let g_mut = borrow_global_mut<Global>(@aptos_framework); |
There was a problem hiding this comment.
I don't understand why you borrow Global with @aptos_framework and you don't have any access restriction, and you get something when you have called move_to with admin?
|
@0xmovses @musitdev see my comment here #227 (comment) |
|
Implemented in the #227 |
Description
Based on MIP-124.
Testing
Code currently compiles
aptos move compile