[FA] s/withdraw/gasburn/ event & s/deposit/gasrefunded/ event #15357

lightmark · 2024-11-21T21:44:47Z

Description

We don't emit event when burn APT coin, neither should APT FA.
Now each txn burn gas fee from APT FA, we see a withdraw event for this.

We issue GasBurnt event and GasRefunded event in both cases instead.

How Has This Been Tested?

simple refactoring.

Type of Change

Which Components or Systems Does This Change Impact?

trunk-io · 2024-11-21T21:44:51Z

⏱️ 27m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-move-tests	13m	🟩
rust-move-tests	7m	🟥
rust-cargo-deny	3m	🟩 🟩
check-dynamic-deps	1m	🟩 🟩
general-lints	1m	🟩 🟩
semgrep/ci	43s	🟩 🟩
file_change_determinator	23s	🟩 🟩
permission-check	6s	🟩 🟩
permission-check	4s	🟩 🟩

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

igor-aptos · 2024-11-21T21:55:09Z

aptos-move/framework/aptos-framework/sources/fungible_asset.move

@@ -898,7 +899,7 @@ module aptos_framework::fungible_asset {
        amount: u64
    ) acquires FungibleStore, Supply, ConcurrentSupply, ConcurrentFungibleBalance {
        // ref metadata match is checked in burn() call
-        burn(ref, withdraw_internal(object::object_address(&store), amount));
+        burn(ref, raw_withdraw_internal(object::object_address(&store), amount));
    }

    public(friend) fun address_burn_from(


I wanted to this irrespective, but since you are making it even more different - let's rename this to address_burn_from_for_gas so it's clear this shouldn't be used anywhere else

igor-aptos · 2024-11-21T21:59:53Z

aptos-move/framework/aptos-framework/sources/fungible_asset.move

@@ -1057,8 +1069,6 @@ module aptos_framework::fungible_asset {
                assert!(store.balance >= amount, error::invalid_argument(EINSUFFICIENT_BALANCE));
                store.balance = store.balance - amount;
            };
-


weren't folks complaining that there wasn't a Withdraw before? after migration now it is consistently to be always there?

also changing this after migration has started seems even more confusing? maybe we should create WithdrawForGas event and emit that one instead, so callers can choose what to do with it?

@movekevin for thoughts

Part of the reason I break so many tests is due to this new event.
I don't know who complained there was not a withdraw before.
cc @kent-white @gregnazario for your thoughts as well.

It seems mint_and_refund also has a deposit event... I am not sure whether we should have this PR now.

can the caller know when (or how much) of the gas fee has been paid by coin (as so has no event) vs fa (and so has this event).
If that cannot be distinguished, then users cannot appropriately adjust the balances. so we should try to address that, in the cleanest way possible

coin gas fee is implicit. I think we can only know from FeeStatement.

lightmark · 2024-11-21T22:57:36Z

I create two events to presents gas burnt and gas refunded.

igor-aptos

cc @runtian-zhou for thoughts here, as you are changing these files for permissions as well

separation here also makes a permission checks more clearer - _for_gas methods have a different permission check (all the way in transaction_validation), and other methods have permission check here. And events here also then distinguish the permission needed, which is cool

cc @movekevin and @davidiw for thoughts on this, as this is pretty big change logically

igor-aptos · 2024-11-22T18:27:37Z

aptos-move/framework/aptos-framework/sources/aptos_account.move

@@ -265,7 +265,7 @@ module aptos_framework::aptos_account {
        // Skip burning if amount is zero. This shouldn't error out as it's called as part of transaction fee burning.


rename burn_from_fungible_store to burn_from_fungible_store_for_gas then

igor-aptos · 2024-11-22T18:28:50Z

aptos-move/framework/aptos-framework/sources/coin.move

@@ -779,9 +779,12 @@ module aptos_framework::coin {
            burn(coin_to_burn, burn_cap);
        };
        if (fa_amount_to_burn > 0) {
-            fungible_asset::burn_from(
+            fungible_asset::address_burn_from_for_gas(


burn_from is a public function that might be called from anywhere? we would need separate public(friend) for_gas variant and leave the public to emit regular events?

igor-aptos · 2024-11-22T18:29:37Z

aptos-move/framework/aptos-framework/sources/fungible_asset.move

@@ -215,6 +215,18 @@ module aptos_framework::fungible_asset {
        frozen: bool,
    }

+    #[event]
+    struct GasBurnt has drop, store {


we are not replacing Burn event, we are replacing Withdraw event, so this should be GasWithdraw or WithdrawForGas, similar below.

runtian-zhou

I don't think this would help the permissions? As there were no signers involved at all. Seems to me a purely functional change for FA

runtian-zhou · 2024-11-22T18:58:16Z

aptos-move/framework/aptos-framework/sources/fungible_asset.move

        let FungibleAsset { metadata, amount } = fa;
        assert!(exists<FungibleStore>(store_addr), error::not_found(EFUNGIBLE_STORE_EXISTENCE));
        let store = borrow_global_mut<FungibleStore>(store_addr);
        assert!(metadata == store.metadata, error::invalid_argument(EFUNGIBLE_ASSET_AND_STORE_MISMATCH));

-        if (amount == 0) return;
+        if (amount != 0) {


can keep amount==0 to save for one indentation.

it's inline.

runtian-zhou · 2024-11-22T19:00:26Z

aptos-move/framework/aptos-framework/sources/fungible_asset.move

@@ -1019,26 +1032,54 @@ module aptos_framework::fungible_asset {
        assert!(amount == 0, error::invalid_argument(EAMOUNT_IS_NOT_ZERO));
    }

-    public(friend) fun deposit_internal(store_addr: address, fa: FungibleAsset) acquires FungibleStore, ConcurrentFungibleBalance {
+    inline fun raw_deposit_internal(


Naming here seems pretty bad. Both raw and internal imply the same thing? I think what we are really referring here is unchecked_deposit_with_no_events?

runtian-zhou · 2024-11-22T19:01:51Z

aptos-move/framework/aptos-framework/sources/fungible_asset.move

-    /// Extract `amount` of the fungible asset from `store`.
-    public(friend) fun withdraw_internal(
+    /// Extract `amount` of the fungible asset from `store` emitting event.
+    public(friend) fun raw_withdraw(


Same for raw_withdraw. I think this is more like unchecked_withdraw?

davidiw · 2024-11-22T19:19:54Z

looks like this could be very expensive perf wise -- are we unable to compute this? why do we need this?

lightmark · 2024-11-22T20:17:06Z

I don't think this would help the permissions? As there were no signers involved at all. Seems to me a purely functional change for FA

it will have signers. I will refactor a little bit after AA code.

lightmark · 2024-11-28T03:30:04Z

looks like this could be very expensive perf wise -- are we unable to compute this? why do we need this?
which is expensive? if you're referring to event, the status quo is we emit an event when burn gas. This PR just replace it with another event.

Also, an exchange asked us how they can tell the difference between migration txn depositing FA or transfer depositing FA. I think this PR can solve their issue.

lightmark requested review from davidiw, movekevin and wrwg as code owners November 21, 2024 21:44

lightmark requested a review from igor-aptos November 21, 2024 21:49

igor-aptos reviewed Nov 21, 2024

View reviewed changes

lightmark force-pushed the lightmark/no_withdraw_event_from_burn_fa branch from 4760bba to ab06352 Compare November 21, 2024 22:55

lightmark changed the title ~~[FA] no withdraw event for burn~~ [FA] s/withdraw/gasburn/ event & s/deposit/gasrefunded/ event Nov 21, 2024

lightmark requested review from kent-white and gregnazario November 22, 2024 16:35

igor-aptos reviewed Nov 22, 2024

View reviewed changes

runtian-zhou reviewed Nov 22, 2024

View reviewed changes

[FA] no withdraw event for burn

a8d769c

lightmark force-pushed the lightmark/no_withdraw_event_from_burn_fa branch from ab06352 to a8d769c Compare December 2, 2024 04:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FA] s/withdraw/gasburn/ event & s/deposit/gasrefunded/ event #15357

[FA] s/withdraw/gasburn/ event & s/deposit/gasrefunded/ event #15357

lightmark commented Nov 21, 2024 •

edited

Loading

trunk-io bot commented Nov 21, 2024 •

edited

Loading

igor-aptos Nov 21, 2024

igor-aptos Nov 21, 2024

lightmark Nov 21, 2024

lightmark Nov 21, 2024

igor-aptos Nov 22, 2024

lightmark Nov 22, 2024

lightmark commented Nov 21, 2024

igor-aptos left a comment

igor-aptos Nov 22, 2024

igor-aptos Nov 22, 2024

igor-aptos Nov 22, 2024

runtian-zhou left a comment

runtian-zhou Nov 22, 2024

lightmark Nov 22, 2024

runtian-zhou Nov 22, 2024

runtian-zhou Nov 22, 2024

davidiw commented Nov 22, 2024

lightmark commented Nov 22, 2024

lightmark commented Nov 28, 2024 •

edited

Loading

		@@ -265,7 +265,7 @@ module aptos_framework::aptos_account {
		// Skip burning if amount is zero. This shouldn't error out as it's called as part of transaction fee burning.

[FA] s/withdraw/gasburn/ event & s/deposit/gasrefunded/ event #15357

Are you sure you want to change the base?

[FA] s/withdraw/gasburn/ event & s/deposit/gasrefunded/ event #15357

Conversation

lightmark commented Nov 21, 2024 • edited Loading

Description

How Has This Been Tested?

Type of Change

Which Components or Systems Does This Change Impact?

trunk-io bot commented Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightmark commented Nov 21, 2024

igor-aptos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runtian-zhou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidiw commented Nov 22, 2024

lightmark commented Nov 22, 2024

lightmark commented Nov 28, 2024 • edited Loading

lightmark commented Nov 21, 2024 •

edited

Loading

trunk-io bot commented Nov 21, 2024 •

edited

Loading

lightmark commented Nov 28, 2024 •

edited

Loading