[framework] Change docs about the usage of FA::transfer, change PFS balance function

[runtian-zhou]

Description

The change also adds documentation notes to several functions in the fungible_asset module, indicating that they can be replaced by their dispatchable counterparts. Additionally, it updates the simple_dispatchable_token test module to include new balance and supply functions.

Change the semantics for pfs::balance to invoke the derived balance function if that's registered.

How Has This Been Tested?

Added tests

Key Areas to Review

Gas, performance

Type of Change

• Refactoring
• Documentation update
• Tests

Which Components or Systems Does This Change Impact?

• Aptos Framework

Checklist

• I have read and followed the CONTRIBUTING doc
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I identified and added all stakeholders and component owners affected by this change as reviewers
• I tested both happy and unhappy path of the functionality
• I have made corresponding changes to the documentation

[trunk-io]

⏱️ 4h 5m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
execution-performance / single-node-performance	1h 42m	🟥 🟥 🟥
forge-e2e-test / forge	45m	🟥 🟥
rust-move-unit-coverage	15m	🟩
rust-move-unit-coverage	15m	🟩
execution-performance / test-target-determinator	13m	🟩 🟩 🟩
rust-move-tests	10m	🟩
rust-move-tests	10m	🟩
rust-move-tests	9m	🟩
test-target-determinator	9m	🟩 🟩
rust-cargo-deny	6m	🟩 🟩 🟩
check-dynamic-deps	6m	🟩 🟩 🟩 🟩
general-lints	2m	🟩 🟩 🟩
semgrep/ci	2m	🟩 🟩 🟩 🟩
file_change_determinator	35s	🟩 🟩 🟩
file_change_determinator	21s	🟩 🟩

🚨 1 job on the last run was significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
execution-performance / single-node-performance	34m	20m

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

[runtian-zhou]

• [framework] Change docs about the usage of FA::transfer, change PFS balance function #14980 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[runtian-zhou]

Thanks for @tyshko5 @mpsc0x @matchv @meng-xu-cs @StErMi for the suggestions!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.1%. Comparing base (c155fde) to head (dd0e76c).
Report is 49 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #14980    +/-   ##
========================================
  Coverage    60.0%    60.1%            
========================================
  Files         856      856            
  Lines      210625   211110   +485     
========================================
+ Hits       126555   126962   +407     
- Misses      84070    84148    +78     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lightmark]

This changed the optimization @igor-aptos wrote.

[runtian-zhou]

Would be curious to see how much optimization it would change here? Seems like they should be the same?

[igor-aptos]

what does dispatchable balance means?

It sounds incompatible with concurrent balance (i.e. being able to do += , -= or >= on balance, without revealing balance itself).

Does that mean that any dispatchable asset cannot be made concurrent?

For assets that don't have registered dispatchable functions, we cannot be revealing balance (in order to keep concurrency), so we would need to have dispatchable_fungible_asset::is_derived_balance_at_least, and use fungible_asset::is_balance_at_least if no dispatchable functions are registered.

we should also confirm APT gas charging (as APT is guaranteed to be non-dispatchable) is still efficient.

[igor-aptos]

cc @georgemitenkov , @gelash for thoughts

[igor-aptos]

@runtian-zhou to clarify:

in fungible_asset.move, is_balance_at_least does:
aggregator_v2::is_at_least(&balance_resource.balance, amount)
and balance does:
aggregator_v2::read(&balance_resource.balance)

aggregator_v2::is_at_least is fully parallel. aggregator_v2::read is fully sequential.

[runtian-zhou]

Looking at the code there I think we might not want to change this. I reverted the relevant lines.

[igor-aptos]

hm, if you revert, does that mean that is_balance_at_least does a wrong thing for dispatchable assets?

[igor-aptos]

back to original question, what does "dispatchable_derived_balance" and "dispatchable_derived_supply" mean? is there an AIP with explanation of those functions?

[runtian-zhou]

There wasn't. That was mostly added as a feature request from our partners.

[runtian-zhou]

In this case, the dispatchable token is_greater_than function won't be parallelizable because we didn't register the hook, so we have to use the balance function to read out the u64 first.

[igor-aptos]

cc @georgemitenkov , @gelash for thoughts

[runtian-zhou]

The tricky thing is the following:

Assets can register a dispatchable function to derive the balance in a coin store. However, what should the pfs::balance show? IMHO the balance derivation is a helpful yet confusing feature because the user will see the raw value anyway from the indexer and onchain explorer, but if they query the dispatched derived balance they will get a different result. Right now the pfs::balance/is_balance_at_least always checks for the underlying balance rather than the derived balance.

[runtian-zhou]

I would incline to keep the current semantics:

• primary_fungible_store::balance will still return the underlying balance of the fungible store. This will align with what the indexer going to show.
• derived_balance can only be accessed via dispatchable_fungible_asset::derived_balance.

[meng-xu-cs]

Chiming in here from a user / builder perspective:

It is seriously confusing to see inconsistent behaviors when a Dispatchable Fungible Asset (DFA) is involved. To be specific:

• when a DFA has a withdraw hook registered

  • primary_fungible_store::withdraw will invoke the hook
  • fungible_asset::withdraw will abort

• when a DFA has a balance hook registered

  • primary_fungible_store::balance will NOT invoke the hook, but will return a "raw" balance which may or may not bear any meaning
  • fungible_asset::withdraw will NOT abort, but will return a "raw" balance which may or may not bear any meaning

This inconsistency is likely to cause confusion to users, wallets, DeFi protocol builders, etc, hurts potential composability of DeFi contracts, and such confusion and incompatibility will be bad for the ecosystem. IMHO, such inconsistency is always better to be resolved earlier than later.

[runtian-zhou]

For withdraw/deposit, I don't really see a clean, safe path for us to get rid of the current constraints. This is mainly because of the following:

1. The fungible_asset.move already exists on chain. The dispatched hook will need to import this module to operate on the fungible assets they manage.
2. When the hook invoke this function, the fungible_asset module cannot exists on top of the call stack.
3. Thus the fungible_asset::transfer cannot exist on top of the call stack, as it could lead to violations in reference/reentrancy safety.

For the balance function, it is indeed very confusing. The overloaded balance is added just cuz we thought it could be an interesting use case and could be used to implement features like inflationary coins over time. With that said, the indexer will always show the underlying "balance" stored on chain which will align with what's shown in the primary_fungible_store::balance. That's the reason why the entry point actually has the name derived_balance to distinguish from the underlying balance. Would love to see your suggestion on how you feel about that.

[meng-xu-cs]

Thanks for the clarification @runtian-zhou. I understand the background and complications. In fact, the comment from me above is NOT about fixing fungible_asset.move, but just to push for a fix on the inconsistency between 'withdraw' and 'balance'.

[runtian-zhou]

Gas charging uses aptos_account::is_fungible_balance_at_least and it should be not affected by this change, unless the flag gets reverted, then the coin::balance will use this api and thus be impacted.

[igor-aptos]

This looks good to me now. Two questions:

should fungible_asset::balance abort if dispatchable hooks are registered?

Will the changes in this PR break any existing code? they change the semantics of some of the functions - that users might have realized to not do the right thing - and then applied workarounds on top of them? specifically the PFS functions

[github-actions]

✅ Forge suite realistic_env_max_load success on 2c7f27fe12d74aa10c2c54cc64a0d7f753021360

two traffics test: inner traffic : committed: 14728.64 txn/s, latency: 2698.19 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3600 ms), latency samples: 5600160
two traffics test : committed: 100.00 txn/s, latency: 1513.06 ms, (p50: 1400 ms, p70: 1400, p90: 2200 ms, p99: 2600 ms), latency samples: 1660
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.588, avg: 1.439", "ConsensusProposalToOrdered: max: 0.292, avg: 0.290", "ConsensusOrderedToCommit: max: 0.314, avg: 0.299", "ConsensusProposalToCommit: max: 0.606, avg: 0.590"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.71s no progress at version 30187 (avg 0.19s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.57s no progress at version 2578092 (avg 0.57s) [limit 16].
Test Ok

• Grafana dashboard
• Humio Logs
• Axiom Logs
• Validator CPU Profile
• Fullnode CPU Profile
• Test runner output
• Test run is land-blocking

[github-actions]

✅ Forge suite compat success on 6593fb81261f25490ffddc2252a861c994234c2a ==> 2c7f27fe12d74aa10c2c54cc64a0d7f753021360

Compatibility test results for 6593fb81261f25490ffddc2252a861c994234c2a ==> 2c7f27fe12d74aa10c2c54cc64a0d7f753021360 (PR)
1. Check liveness of validators at old version: 6593fb81261f25490ffddc2252a861c994234c2a
compatibility::simple-validator-upgrade::liveness-check : committed: 18242.79 txn/s, latency: 1923.38 ms, (p50: 2000 ms, p70: 2100, p90: 2100 ms, p99: 2300 ms), latency samples: 584400
2. Upgrading first Validator to new version: 2c7f27fe12d74aa10c2c54cc64a0d7f753021360
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7939.64 txn/s, latency: 3713.81 ms, (p50: 4200 ms, p70: 4500, p90: 4900 ms, p99: 5000 ms), latency samples: 143220
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 8034.04 txn/s, latency: 4229.11 ms, (p50: 4600 ms, p70: 4700, p90: 4900 ms, p99: 5100 ms), latency samples: 268240
3. Upgrading rest of first batch to new version: 2c7f27fe12d74aa10c2c54cc64a0d7f753021360
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7713.10 txn/s, latency: 3884.46 ms, (p50: 4400 ms, p70: 4700, p90: 4800 ms, p99: 4900 ms), latency samples: 143240
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7759.40 txn/s, latency: 4371.83 ms, (p50: 4800 ms, p70: 4900, p90: 5000 ms, p99: 5300 ms), latency samples: 257180
4. upgrading second batch to new version: 2c7f27fe12d74aa10c2c54cc64a0d7f753021360
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 12141.44 txn/s, latency: 2412.27 ms, (p50: 2700 ms, p70: 2900, p90: 3000 ms, p99: 3000 ms), latency samples: 208540
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 12054.48 txn/s, latency: 2730.87 ms, (p50: 2900 ms, p70: 3000, p90: 3100 ms, p99: 3300 ms), latency samples: 391600
5. check swarm health
Compatibility test for 6593fb81261f25490ffddc2252a861c994234c2a ==> 2c7f27fe12d74aa10c2c54cc64a0d7f753021360 passed
Test Ok

• Grafana dashboard
• Humio Logs
• Axiom Logs
• Validator CPU Profile
• Fullnode CPU Profile
• Test runner output
• Test run is land-blocking