update rewards and governed pool MIP-124 - #227
Conversation
This reverts commit 8b72cd3. Too complexe, we use the Aptos way to update reward rate.
Ok I revert the commit and do the test to confirm the issue. |
0xmovses
changed the title
|
All unit tests are passing. Only ask is that we update the Move fmt. And then @areshand can review. Thanks! |
| amount: u64 | ||
| ): Coin<CoinType> acquires GovernedGasPool { | ||
| let s = governed_gas_signer(); // uses the private signer function | ||
| coin::withdraw<CoinType>(&s, amount) |
There was a problem hiding this comment.
can we have a dedicated event for withdraw from GGP for accounting purpose
There was a problem hiding this comment.
Add an event to GGP is a little more complicated than expected. Creating the event handler in the unit test need to have the sender account created, and the framework account is not for most of the test that use the staking.
I've tested, and I get the MISSING_DATA (code 4008) error when the handler is created.
To solve this, you need to add this line in all test that fails: aptos_framework::account::create_account_for_test(@aptos_framework);
I've tested using account::new_event_handle<>() and event::new_event_handle<>() , the account must be created to create the handler.
What do you think, I add the create_account_for_test in all failing test (all test that use the GGP) ?
There was a problem hiding this comment.
I push the account implementation with 2 tests updated.
There was a problem hiding this comment.
That's correct, it uses the resource account pattern https://aptos.dev/build/smart-contracts/resource-accounts
What do you think, I add the create_account_for_test in all failing test (all test that use the GGP) ?
It might seem clunky but it's correct, so I would say yes.
areshand
left a comment
areshand
left a comment
There was a problem hiding this comment.
Just in case we want to shut down this part of logic
|
|
||
| // Decrease the treasury counter. | ||
| if (ggp.deposited_treasury_counter > amount) { | ||
| ggp.deposited_treasury_counter = ggp.deposited_treasury_counter - amount; |
There was a problem hiding this comment.
maybe we rename deposited_treasury_counter to total_deposited_treasury_counter. we don't need to decrease it
There was a problem hiding this comment.
In fact, what is the goal of this treasury counter. If it's to know how many treasures that was deposited, it shouldn't be decrease, if it's the part of the deposited treasure in the current pool, we should decrease it.
Perhaps by adding an event to deposit_treasury we can manage these data off chain and not on chain.
There was a problem hiding this comment.
IMO, we can only know how many treasures that was deposited. During withdraw reward, we dont differentiate gas fund from treasury fund
There was a problem hiding this comment.
So the need is to know how many treasure has been deposited. I remove the withdraw.
There was a problem hiding this comment.
I pull the change, the treasure decrease has been removed.
| } | ||
|
|
||
| /// Contains added variable needed for the GovernedGasPool staking reward update. | ||
| struct GovernedGasPoolV2 has key { |
There was a problem hiding this comment.
naming is hard. But this is not ggp v2. this is more like a GovernedGasPoolExtension.
| signer_capability: governed_gas_pool_signer_cap, | ||
| }); | ||
|
|
||
| move_to(aptos_framework, GovernedGasPoolV2{ |
There was a problem hiding this comment.
this is only called during genesis. You need to a separate function to initialize this resource. Otherwise, deposit and withdraw would break
|
Note: Partner PR to follow, this squash commit into |
This was cherry picked from `0ae9d6f8721954e7428a9feefac83ae52bff0355` on `movement-migration` which is the squash commit from PR #227. We need this commits now on `movement` for testing purposes. --------- Co-authored-by: Bo Wu <bo.wu@movementlabs.xyz> Co-authored-by: musitdev <philippe.delrieu@free.fr>
Based on [MIP-124](movement-network/MIP#124). Implements this changes: https://www.notion.so/movementlabs/Staking-reward-using-GGP-28118675b2d780848e30c0c7e51d47cc In the folder `aptos-move/framework/aptos-framework` run `movement move test` to run all test. Or `movement move test -f stake `to run stake related tests. --------- Co-authored-by: musitdev <philippe.delrieu@free.fr>
Based on [MIP-124](movement-network/MIP#124). Implements these changes: https://www.notion.so/movementlabs/Staking-reward-using-GGP-28118675b2d780848e30c0c7e51d47cc In the folder `aptos-move/framework/aptos-framework` run `aptos move test` to run all test. Or `aptos move test -f stake `to run stake related tests. --------- See the #227 original PR --------- Co-authored-by: musitdev <philippe.delrieu@free.fr>
4 participants
Description
Based on MIP-124.
Implements this changes: https://www.notion.so/movementlabs/Staking-reward-using-GGP-28118675b2d780848e30c0c7e51d47cc
Testing
In the folder
aptos-move/framework/aptos-frameworkrunmovement move testto run all test.Or
movement move test -f staketo run stake related tests.