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

[fungible_asset] a layered design #7183

Merged
merged 20 commits into from
Mar 24, 2023
Merged

[fungible_asset] a layered design #7183

merged 20 commits into from
Mar 24, 2023

Conversation

lightmark
Copy link
Contributor

Description

This defines fungibility or a fungible asset in Aptos Move.

  • At the core is an asset object equipped with FungibleSource that defines asset metadata(fixed supply only for now) and during creation provides minting, burning, and freezing capabilities. So FungibleSource entitles an asset object with fungibility.
  • A FungibleSource's mint, burn, and freeze capabilities manage FungibleAssets, which contain a balance and point back to the originating asset object that has FungibleSource.

As an example, this also introduces a ManagedFungibleSource that enables the owner of the asset object with fungible source to mint, burn, and freeze these FungibleAssets.

Test Plan

unit test.

@@ -15,5 +15,5 @@ module aptos_framework::create_signer {
friend aptos_framework::multisig_account;
friend aptos_framework::object;

public(friend) native fun create_signer(addr: address): signer;
public native fun create_signer(addr: address): signer;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please ignore for now but I need this for the creation of FungibleAssetIndex and FungibleAsset object with deposit from nowhere.

I suppose we'll merge this package into AptosFramework sooner or later so it's okay to use this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why we need to share the signer... if we jump to this, we're missing a permissions model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we are missing the permission model. I don't mean this is the final change. It is just a hack for me to test. The way we jump to this is:

  1. make FA module in the framework.
  2. friend FA module here and keeep public(friend)

I need this to create a FA object from nowhere when getting money the first time. It's for unilateral transfer.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, to create a new object we need a signer. When we're sending coins to someone for the first time and they don't have the object to store them, there's not a perfect signer to use (using sender's signer can be strange)

@lightmark lightmark force-pushed the lightmark/fungible_asset branch from 2a1e180 to bcaefe1 Compare March 15, 2023 22:02
@lightmark lightmark force-pushed the lightmark/fungible_asset branch from bcaefe1 to 4f55b75 Compare March 15, 2023 22:04
@lightmark lightmark marked this pull request as ready for review March 15, 2023 23:33
Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

amazing, amazing

@@ -15,5 +15,5 @@ module aptos_framework::create_signer {
friend aptos_framework::multisig_account;
friend aptos_framework::object;

public(friend) native fun create_signer(addr: address): signer;
public native fun create_signer(addr: address): signer;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why we need to share the signer... if we jump to this, we're missing a permissions model.

/// Mint the `amount` of coin with MintCap.
public fun mint_with_cap<T: key>(
cap: &MintCap,
asset: &Object<T>,
Copy link
Contributor

Choose a reason for hiding this comment

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

the cap should sufficiently define the asset, no need for both or we have an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should call this mint_into?
how does one create a new object, can anyone do that? I need to read more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only the create_signer I hacked can create a new FA object... But you dislike it for no reason :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Option 1) Use the extendref that comes with the source, option 2) pass in the entity who will own the object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That totally breaks the current code structure. Because when you deposit a hot potato FA into my account, you don't want me to provide my signer. This blocks unilateral sending.

@lightmark
Copy link
Contributor Author

lightmark commented Mar 17, 2023

3 questions to answer:

  1. how to create object from nowhere, use create_signer from framework?
  2. do we want to delete the PinnedFungibleAsset Object if the balance depletes? (omg I hate this name...~)
  3. how to deal with coin? My feeling is we may want to do what I showed in node_code_coin.move

@lightmark lightmark requested review from 0xchloe and CapCap March 17, 2023 22:16
Comment on lines 124 to 133
/// Freeeze/unfreeze any account of asset address `asset_addr`.
public(friend) fun set_frozen_flag(
fungible_asset_owner: address,
asset_addr: address,
frozen: bool
) acquires FungibleAssetStore, PinnedFungibleAsset {
let pfa_opt = get_pinned_fungible_asset_object(fungible_asset_owner, asset_addr, true);
let pfa = option::destroy_some(pfa_opt);
borrow_global_mut<PinnedFungibleAsset>(object_address(&pfa)).frozen = frozen;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

we're going down a dangerous path here, we should push the concept of the TransferRef into here rather than re-invent.. then fungible assets could determine what it means to freeze, enable allowlists, partial freeze, approvals, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand what you mean here. But it is really mind-blowing that how to offer the flexibility of all the features you mentioned here. lemme think about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't overthink it, we already have the TransferRef pattern and ungated transfers, just replicate it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only for partial freeze. I am confused that how transferRef pattern deal with this case.

Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

I think we should merge fungible_caps, fungible_source, and fungible_assets. I'm happy to contribute!

Comment on lines 7 to 8
#[test_only]
use aptos_framework::fungible_source::init_test_fungible_source;
Copy link
Contributor

Choose a reason for hiding this comment

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

let's be consistent here and have this where it is used and we're importing a function 😭

balance: u64,
/// Fungible Assets transferring is a common operation, this allows for disabling and enabling
/// transfers bypassing the use of a TransferCap.
allow_ungated_transfer: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

oh this is interesting... you can have a gated withdraw and a gated deposit....


#[resource_group_member(group = aptos_framework::object::ObjectGroup)]
/// The resource of an object recording the properties of the fungible assets held of the object owner.
struct AccountFungibleAsset has key {
Copy link
Contributor

Choose a reason for hiding this comment

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

gaah, no no, FungibleAssetResource or just FungibleResource

AccountFungibleAsset is the framework that let's a user specify their primary one.

}

/// Return the underlying fungible source.
public fun fungible_asset_source(fa: &FungibleAsset): Object<FungibleSource> {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's not add fungible_asset to each of these, we should move against this double namespacing.

const ETRANSFER_CAP_AND_FUNGIBLE_ASSET_MISMATCH: u64 = 1;

/// Capability to mint fungible asset.
struct MintCap has store {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use Ref per previous convos
We also use drop everywhere 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... you stressed the two things I hate.

@lightmark
Copy link
Contributor Author

@davidiw fungible_caps has to use fungible_store to transfer fas. And fungible store depends on fungible_asset. There is no way to isolate fungible_store and merge the other three.

Basically if you have caps, you can burn from any account and transfer from/to any account so you must be aware of the fungible store.

@davidiw
Copy link
Contributor

davidiw commented Mar 22, 2023

@davidiw fungible_caps has to use fungible_store to transfer fas. And fungible store depends on fungible_asset. There is no way to isolate fungible_store and merge the other three.

Basically if you have caps, you can burn from any account and transfer from/to any account so you must be aware of the fungible store.

Option 1) land as is
Option 2) I tweak and we land after I tweak

preference?

I'm going with Option 2... I have a new found appreciation for the hell you have been working on :)

@lightmark
Copy link
Contributor Author

lightmark commented Mar 23, 2023

@davidiw the refactoring is done.

}

/// Ensure the existence and return the `FungibleAssetWallet`.
inline fun ensure_fungible_asset_wallet(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is outdated as it's creating FungibleAssetStore not fungible_asset_wallet

Comment on lines 215 to 219
/// Create a default `FungibleAssetWallet` object with zero balance of `metadata`.
fun create_account_fungible_asset_object(
account: address,
metadata: &Object<FungibleAssetMetadata>
): Object<FungibleAssetWallet> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should restrict what is a fungible asset (wallet) and what isn't

I'd prefer we just allow someone to pass in a constructor ref and to their thing

this should be parallelizable -- otherwise account creation will become serial

/// Remove the `FungibleAssetWallet` object of `metadata` from `account` if balance drops to 0 and
/// `allowed_ungated_transfer` is allowed.
inline fun maybe_delete(wallet: Object<FungibleAssetWallet>) acquires FungibleAssetStore {
if (fungible_asset::balance(&wallet) == 0 && fungible_asset::ungated_transfer_allowed(&wallet)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's revisit the goal here, I don't think we just want to delete something if it is empty
consider the case where it is your primary wallet, I think that lifetime should be managed by the fungible_store

Copy link
Contributor Author

@lightmark lightmark Mar 24, 2023

Choose a reason for hiding this comment

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

This is something I am not quite sure. I can remove it for now, but also keep in mind that you may not want the wallets with 0 balance of different metadata objects that you just used once.


#[resource_group_member(group = aptos_framework::object::ObjectGroup)]
/// Hold refs to control the minting, transfer and burning of fungible assets.
struct ManagingRefs has key {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's be bland here and call this ManagedAsset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have an opinion here.

assert_owner(metadata_owner, metadata);
let mint_ref = borrow_mint_from_refs(metadata);
let fa = fungible_asset::mint(mint_ref, amount);
fungible_store::deposit(fa, to);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this module should know about fungible store

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's quite different than I thought. Then basically we have to build another layer otherwise most users will reinvent the wheel. Agree?

@@ -0,0 +1,48 @@
/// This is an example showing how to create a fungible asset and how to use it.
module fungible_asset::coin {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this exist purely for demos and testing? I think this can be incorporated as an integration test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

showing ppl how to use it as a demo

Comment on lines 39 to 71
/// Return true if `account` allows ungated transfer.
public fun ungated_transfer_allowed<T: key>(
account: address,
metadata: &Object<T>
): bool acquires FungibleAssetStore {
let metadata = fungible_asset::verify(metadata);
let afa_opt = get_account_fungible_asset_object(
account,
&metadata,
false
);
if (option::is_none(&afa_opt)) {
return true
};
let wallet = option::destroy_some(afa_opt);
fungible_asset::ungated_transfer_allowed(&wallet)
}

/// Enable/disable the direct transfer.
public fun set_ungated_transfer(
ref: &TransferRef,
account: address,
allow: bool
) acquires FungibleAssetStore {
let metadata = fungible_asset::verify(&fungible_asset::transfer_ref_metadata(ref));
let afa_opt = get_account_fungible_asset_object(account, &metadata, !allow);
if (option::is_none(&afa_opt)) {
return
};
let wallet = option::destroy_some(afa_opt);
fungible_asset::set_ungated_transfer(ref, &wallet, allow);
maybe_delete(wallet);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for this store, we should just expose the most trivial operations, let's consider deleting these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we delete this, users have to implement all those by themselves?

@davidiw
Copy link
Contributor

davidiw commented Mar 24, 2023

There's a lot of work that needs to be done here before this is ready for 1.4 cut.

}

/// Destroy `wallet` object.
public(friend) fun destory_fungible_asset_wallet(
Copy link
Contributor

Choose a reason for hiding this comment

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

destory => destroy

@movekevin movekevin enabled auto-merge (squash) March 24, 2023 18:05
/// Define the metadata required of an metadata to be fungible.
struct FungibleAssetMetadata has key {
/// The current supply.
supply: u64,
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we use u128

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

✅ Forge suite land_blocking success on 9a3f3fccc67f5f4576643690f1f7b7ce0b806e0e

performance benchmark with full nodes : 5696 TPS, 6964 ms latency, 9900 ms p99 latency,no expired txns
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 9a3f3fccc67f5f4576643690f1f7b7ce0b806e0e

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 9a3f3fccc67f5f4576643690f1f7b7ce0b806e0e (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7863 TPS, 4896 ms latency, 7200 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 9a3f3fccc67f5f4576643690f1f7b7ce0b806e0e
compatibility::simple-validator-upgrade::single-validator-upgrade : 5184 TPS, 7838 ms latency, 11000 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 9a3f3fccc67f5f4576643690f1f7b7ce0b806e0e
compatibility::simple-validator-upgrade::half-validator-upgrade : 4598 TPS, 8627 ms latency, 11200 ms p99 latency,no expired txns
4. upgrading second batch to new version: 9a3f3fccc67f5f4576643690f1f7b7ce0b806e0e
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6466 TPS, 5983 ms latency, 10500 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 9a3f3fccc67f5f4576643690f1f7b7ce0b806e0e passed
Test Ok

@movekevin movekevin merged commit 7c4a010 into main Mar 24, 2023
@movekevin movekevin deleted the lightmark/fungible_asset branch March 24, 2023 19:07
@github-actions
Copy link
Contributor

❌ Forge suite framework_upgrade failure on cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> 9a3f3fccc67f5f4576643690f1f7b7ce0b806e0e

Compatibility test results for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> 9a3f3fccc67f5f4576643690f1f7b7ce0b806e0e (PR)
Upgrade the nodes to version: 9a3f3fccc67f5f4576643690f1f7b7ce0b806e0e
Test Failed: API error: Unknown error Transaction committed on chain, but failed execution: Transaction Executed and Committed with Error CONSTRAINT_NOT_SATISFIED
Trailing Log Lines:
INCLUDING DEPENDENCY MoveStdlib
BUILDING ExecuteProposal
::error::API error: Unknown error Transaction committed on chain, but failed execution: Transaction Executed and Committed with Error CONSTRAINT_NOT_SATISFIED
test framework_upgrade::framework-upgrade ... FAILED
Error: API error: Unknown error Transaction committed on chain, but failed execution: Transaction Executed and Committed with Error CONSTRAINT_NOT_SATISFIED
Test Statistics: 
Compatibility test results for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> 9a3f3fccc67f5f4576643690f1f7b7ce0b806e0e (PR)
Upgrade the nodes to version: 9a3f3fccc67f5f4576643690f1f7b7ce0b806e0e
Test Failed: API error: Unknown error Transaction committed on chain, but failed execution: Transaction Executed and Committed with Error CONSTRAINT_NOT_SATISFIED


Swarm logs can be found here: See fgi output for more information.
{"level":"INFO","source":{"package":"aptos_forge","file":"testsuite/forge/src/backend/k8s/cluster_helper.rs:281"},"thread_name":"main","hostname":"forge-framework-upgrade-pr-7183-1679683646-cb4ba0a57c998c60cbab","timestamp":"2023-03-24T19:09:24.482886Z","message":"Deleting namespace forge-framework-upgrade-pr-7183: Some(NamespaceStatus { phase: Some(\"Terminating\") })"}
{"level":"INFO","source":{"package":"aptos_forge","file":"testsuite/forge/src/backend/k8s/cluster_helper.rs:389"},"thread_name":"main","hostname":"forge-framework-upgrade-pr-7183-1679683646-cb4ba0a57c998c60cbab","timestamp":"2023-03-24T19:09:24.482911Z","message":"aptos-node resources for Forge removed in namespace: forge-framework-upgrade-pr-7183"}

failures:
    framework_upgrade::framework-upgrade

test result: FAILED. 0 passed; 1 failed; 0 filtered out

Failed to run tests:
Tests Failed
Error: Tests Failed
Debugging output:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants