From be4f8d2cc9913b22318ac0d703cb2eb345cd5f71 Mon Sep 17 00:00:00 2001 From: Yotam Katznelson Date: Sun, 5 Nov 2023 02:51:03 +0100 Subject: [PATCH] [move] fix lifetime burn info (#85) Co-authored-by: 0o-de-lally <1364012+0o-de-lally@users.noreply.github.com> --- .../sources/modified_source/coin.move | 16 ++++++-- .../sources/ol_sources/burn.move | 31 +++++++++----- .../sources/ol_sources/epoch_boundary.move | 6 ++- .../sources/ol_sources/pledge_accounts.move | 3 +- .../sources/ol_sources/proof_of_fee.move | 2 +- .../sources/ol_sources/tests/burn.test.move | 4 +- .../ol_sources/tests/donor_directed.test.move | 41 +++++++++++++------ .../ol_sources/tests/rewards.test.move | 8 +--- 8 files changed, 71 insertions(+), 40 deletions(-) diff --git a/framework/libra-framework/sources/modified_source/coin.move b/framework/libra-framework/sources/modified_source/coin.move index 1632885a3..7e584976c 100644 --- a/framework/libra-framework/sources/modified_source/coin.move +++ b/framework/libra-framework/sources/modified_source/coin.move @@ -18,6 +18,7 @@ module diem_framework::coin { // use diem_std::debug::print; friend ol_framework::gas_coin; + friend ol_framework::burn; friend ol_framework::ol_account; friend diem_framework::genesis; friend diem_framework::genesis_migration; @@ -341,7 +342,10 @@ module diem_framework::coin { // Public functions /// Burn `coin` with capability. - /// The capability `_cap` should be passed as a reference to `BurnCapability`. + /// The capability `_cap` should be passed as a reference to + /// `BurnCapability`. + + // 0L: TODO: isn't this deprecated? public(friend) fun burn( coin: Coin, _cap: &BurnCapability, @@ -358,8 +362,9 @@ module diem_framework::coin { //////// 0L //////// /// user can burn own coin they are holding. - /// TODO: evaluate if this should be a `friend` method - public fun user_burn( + /// must be a Friend function, because the relevant tracking + /// happens in Burn.move + public(friend) fun user_burn( coin: Coin, ) acquires CoinInfo { // cannot abort if (value(&coin) == 0) { @@ -380,7 +385,10 @@ module diem_framework::coin { /// The capability `burn_cap` should be passed as a reference to `BurnCapability`. /// This function shouldn't fail as it's called as part of transaction fee burning. /// - /// Note: This bypasses CoinStore::frozen -- coins within a frozen CoinStore can be burned. + /// Note: This bypasses CoinStore::frozen -- coins within a frozen CoinStore + /// can be burned. + + // 0L: is this deprecated? public(friend) fun burn_from( account_addr: address, amount: u64, diff --git a/framework/libra-framework/sources/ol_sources/burn.move b/framework/libra-framework/sources/ol_sources/burn.move index f73bc8e37..678f4626a 100644 --- a/framework/libra-framework/sources/ol_sources/burn.move +++ b/framework/libra-framework/sources/ol_sources/burn.move @@ -95,12 +95,12 @@ module ol_framework::burn { i = i + 1; }; - // // Transaction fee account should be empty at the end of the epoch - // // Superman 3 decimal errors. https://www.youtube.com/watch?v=N7JBXGkBoFc - // // anything that is remaining should be burned + // Transaction fee account should be empty at the end of the epoch + // Superman 3 decimal errors. https://www.youtube.com/watch?v=N7JBXGkBoFc + // anything that is remaining should be burned let remainder = coin::value(coins); let leftover = coin::extract(coins, remainder); - coin::user_burn(leftover); + burn_and_track(leftover); // Note: we are still retruning an empty coin to be destroyed by the caller } @@ -122,6 +122,18 @@ module ol_framework::burn { } + /// performs a burn and increments the tracker + /// NOTE: this is unchecked, any user can perform this. + /// the user should call this function and not burn methods on coin.move + /// since those methods do not track the lifetime_burned + public fun burn_and_track(coin: Coin) acquires BurnCounter { + let value_sent = coin::value(&coin); + let state = borrow_global_mut(@ol_framework); + coin::user_burn(coin); + state.lifetime_burned = state.lifetime_burned + value_sent; + } + + /// performs a burn or recycle according to the attributed user's preference public fun burn_with_user_preference( vm: &signer, payer: address, user_share: Coin ) acquires BurnCounter, UserBurnPreference { @@ -135,16 +147,13 @@ module ol_framework::burn { // update the root state tracker let state = borrow_global_mut(@ol_framework); state.lifetime_recycled = state.lifetime_recycled + value_sent; - - coin::user_burn(user_share); - return } }; - // Superman 3 - let state = borrow_global_mut(@ol_framework); - state.lifetime_burned = state.lifetime_burned + value_sent; - coin::user_burn(user_share); + // do a default burn which is not attributed + // also checks for Superman 3 + burn_and_track(user_share); + } diff --git a/framework/libra-framework/sources/ol_sources/epoch_boundary.move b/framework/libra-framework/sources/ol_sources/epoch_boundary.move index 6a80b674f..b7316b688 100644 --- a/framework/libra-framework/sources/ol_sources/epoch_boundary.move +++ b/framework/libra-framework/sources/ol_sources/epoch_boundary.move @@ -230,8 +230,10 @@ module diem_framework::epoch_boundary { // remainder gets burnt according to fee maker preferences burn::epoch_burn_fees(root, &mut all_fees); - // there might be some dust, that should get burned - coin::user_burn(all_fees); + + // coin can finally be destroyed. Up to here we have been extracting from a mutable. + // It's possible there might be some dust, that should get burned + burn::burn_and_track(all_fees); }; compliant_vals diff --git a/framework/libra-framework/sources/ol_sources/pledge_accounts.move b/framework/libra-framework/sources/ol_sources/pledge_accounts.move index 3b8b2c594..cfd77a1d1 100644 --- a/framework/libra-framework/sources/ol_sources/pledge_accounts.move +++ b/framework/libra-framework/sources/ol_sources/pledge_accounts.move @@ -47,6 +47,7 @@ use ol_framework::gas_coin::LibraCoin as GasCoin; use ol_framework::ol_account; use ol_framework::epoch_helper; + use ol_framework::burn; use diem_framework::system_addresses; use diem_framework::coin::{Self, Coin}; @@ -466,7 +467,7 @@ if (is_burn && option::is_some(&c)) { let burn_this = option::extract(&mut c); - coin::user_burn(burn_this); + burn::burn_and_track(burn_this); option::destroy_none(c); } else if (option::is_some(&c)) { let refund_coin = option::extract(&mut c); diff --git a/framework/libra-framework/sources/ol_sources/proof_of_fee.move b/framework/libra-framework/sources/ol_sources/proof_of_fee.move index edbc41683..e1d8bb23d 100644 --- a/framework/libra-framework/sources/ol_sources/proof_of_fee.move +++ b/framework/libra-framework/sources/ol_sources/proof_of_fee.move @@ -90,7 +90,7 @@ module ol_framework::proof_of_fee { // from supply data. public fun genesis_migrate_reward(vm: &signer, nominal_reward: u64) acquires ConsensusReward { - if (signer::address_of(vm) != @ol_framework) return; + system_addresses::assert_ol(vm); // either 0x1 or 0x0 let state = borrow_global_mut(@ol_framework); state.nominal_reward = nominal_reward; diff --git a/framework/libra-framework/sources/ol_sources/tests/burn.test.move b/framework/libra-framework/sources/ol_sources/tests/burn.test.move index d963c74cd..b7b6b96e9 100644 --- a/framework/libra-framework/sources/ol_sources/tests/burn.test.move +++ b/framework/libra-framework/sources/ol_sources/tests/burn.test.move @@ -2,7 +2,7 @@ #[test_only] module ol_framework::test_burn { use ol_framework::mock; - use ol_framework::gas_coin::{Self, LibraCoin as GasCoin}; + use ol_framework::gas_coin; use ol_framework::ol_account; use ol_framework::match_index; use ol_framework::burn; @@ -27,7 +27,7 @@ module ol_framework::test_burn { let alice_burn = 5; let c = ol_account::withdraw(alice, alice_burn); - coin::user_burn(c); + burn::burn_and_track(c); let supply = gas_coin::supply(); assert!(supply == (supply_pre - alice_burn), 7357001); diff --git a/framework/libra-framework/sources/ol_sources/tests/donor_directed.test.move b/framework/libra-framework/sources/ol_sources/tests/donor_directed.test.move index 83585cace..897582037 100644 --- a/framework/libra-framework/sources/ol_sources/tests/donor_directed.test.move +++ b/framework/libra-framework/sources/ol_sources/tests/donor_directed.test.move @@ -304,8 +304,11 @@ module ol_framework::test_donor_directed { let vals = mock::genesis_n_vals(root, 5); // need to include eve to init funds mock::ol_initialize_coin_and_fund_vals(root, 100000, true); - // start at epoch 1, since turnout tally needs epoch info, and 0 may cause issues + // start at epoch 1, since turnout tally needs epoch info, and 0 may cause + // issues mock::trigger_epoch(root); + // a burn happend in the epoch above, so let's compare it to end of epoch + let (lifetime_burn_pre, _) = burn::get_lifetime_tracker(); let (resource_sig, _cap) = ol_account::ol_create_resource_account(alice, b"0x1"); let donor_directed_address = signer::address_of(&resource_sig); @@ -322,8 +325,9 @@ module ol_framework::test_donor_directed { let is_donor = donor_directed_governance::check_is_donor(donor_directed_address, signer::address_of(eve)); assert!(is_donor, 7357002); - // Dave will also be a donor, with half the amount of what Eve sends - ol_account::transfer(dave, donor_directed_address, 21); + // Dave will also be a donor, with half the amount of what Eve + let dave_donation = 21; + ol_account::transfer(dave, donor_directed_address, dave_donation); let is_donor = donor_directed_governance::check_is_donor(donor_directed_address, signer::address_of(dave)); assert!(is_donor, 7357003); @@ -352,9 +356,9 @@ module ol_framework::test_donor_directed { let superman_3 = 1; // rounding from fixed_point32 assert!((*eve_donation_pro_rata + superman_3) == eve_donation, 7357008); - let eve_donation_pro_rata = vector::borrow(&refunds, 1); + let dave_donation_pro_rata = vector::borrow(&refunds, 1); let superman_3 = 1; // rounding from fixed_point32 - assert!((*eve_donation_pro_rata + superman_3) == 21, 7357009); + assert!((*dave_donation_pro_rata + superman_3) == dave_donation, 7357009); let (_, program_balance_pre) = ol_account::balance(donor_directed_address); @@ -368,16 +372,17 @@ module ol_framework::test_donor_directed { assert!(program_balance < program_balance_pre, 7357010); assert!(program_balance == 0, 7357011); + // eve shoul have received funds back assert!(eve_balance > eve_balance_pre, 7357010); - let (lifetime_burn, _lifetime_match) = burn::get_lifetime_tracker(); + let (lifetime_burn_now, _) = burn::get_lifetime_tracker(); // nothing should have been burned, it was a refund - assert!(lifetime_burn == 0, 7357011); + assert!(lifetime_burn_now == lifetime_burn_pre, 7357011); } #[test(root = @ol_framework, alice = @0x1000a, dave = @0x1000d, eve = @0x1000e)] - fun dd_liquidate_to_community(root: &signer, alice: &signer, dave: &signer, eve: &signer) { + fun dd_liquidate_to_match_index(root: &signer, alice: &signer, dave: &signer, eve: &signer) { // Scenario: // Alice creates a donor directed account where Alice, Bob and Carol, are admins. // Dave and Eve make a donation and so are able to have some voting on that account. @@ -387,6 +392,8 @@ module ol_framework::test_donor_directed { mock::ol_initialize_coin_and_fund_vals(root, 100000, true); // start at epoch 1, since turnout tally needs epoch info, and 0 may cause issues mock::trigger_epoch(root); + // a burn happend in the epoch above, so let's compare it to end of epoch + let (lifetime_burn_pre, _) = burn::get_lifetime_tracker(); let (resource_sig, _cap) = ol_account::ol_create_resource_account(alice, b"0x1"); let donor_directed_address = signer::address_of(&resource_sig); @@ -404,7 +411,8 @@ module ol_framework::test_donor_directed { assert!(is_donor, 7357002); // Dave will also be a donor, with half the amount of what Eve sends - ol_account::transfer(dave, donor_directed_address, 21); + let dave_donation = 21; + ol_account::transfer(dave, donor_directed_address, dave_donation); let is_donor = donor_directed_governance::check_is_donor(donor_directed_address, signer::address_of(dave)); assert!(is_donor, 7357003); @@ -423,18 +431,21 @@ module ol_framework::test_donor_directed { let list = donor_directed::get_liquidation_queue(); assert!(vector::length(&list) > 0, 7357005); + // check the table of refunds + // dave and eve should be receiving let (addrs, refunds) = donor_directed::get_pro_rata(donor_directed_address); assert!(*vector::borrow(&addrs, 0) == @0x1000e, 7357006); assert!(*vector::borrow(&addrs, 1) == @0x1000d, 7357007); + // for the first refund, let eve_donation_pro_rata = vector::borrow(&refunds, 0); let superman_3 = 1; // rounding from fixed_point32 assert!((*eve_donation_pro_rata + superman_3) == eve_donation, 7357008); - let eve_donation_pro_rata = vector::borrow(&refunds, 1); + let dave_donation_pro_rata = vector::borrow(&refunds, 1); let superman_3 = 1; // rounding from fixed_point32 - assert!((*eve_donation_pro_rata + superman_3) == 21, 7357009); + assert!((*dave_donation_pro_rata + superman_3) == dave_donation, 7357009); let (_, program_balance_pre) = ol_account::balance(donor_directed_address); let (_, eve_balance_pre) = ol_account::balance(@0x1000e); @@ -447,11 +458,15 @@ module ol_framework::test_donor_directed { assert!(program_balance < program_balance_pre, 7357010); assert!(program_balance == 0, 7357011); + // eve's account should not have changed. + // she does not get a refund, and the policy of the + // program says it goes to the matching index. assert!(eve_balance == eve_balance_pre, 7357010); - let (lifetime_burn, _lifetime_match) = burn::get_lifetime_tracker(); + let (lifetime_burn_now, _lifetime_match) = burn::get_lifetime_tracker(); + // nothing should have been burned, it was a refund - assert!(lifetime_burn == 0, 7357011); + assert!(lifetime_burn_now == lifetime_burn_pre, 7357011); } } diff --git a/framework/libra-framework/sources/ol_sources/tests/rewards.test.move b/framework/libra-framework/sources/ol_sources/tests/rewards.test.move index 8c07e27a7..92eddc0ce 100644 --- a/framework/libra-framework/sources/ol_sources/tests/rewards.test.move +++ b/framework/libra-framework/sources/ol_sources/tests/rewards.test.move @@ -1,15 +1,11 @@ #[test_only] module ol_framework::test_rewards { - #[test_only] use ol_framework::rewards; - #[test_only] use ol_framework::gas_coin::{Self, LibraCoin as GasCoin}; - #[test_only] use diem_framework::coin; - #[test_only] use ol_framework::mock; - #[test_only] use diem_framework::stake; + use ol_framework::burn; #[test(root = @ol_framework)] fun test_pay_reward_single(root: signer) { @@ -58,6 +54,6 @@ module ol_framework::test_rewards { let b = coin::balance(dave); assert!(b == 1000, 7357002); - coin::user_burn(new_coin); + burn::burn_and_track(new_coin); } } \ No newline at end of file