fungible asset improvements for concurrent balance #15836
Conversation
igor-aptos
requested review from
junkil-park,
davidiw,
movekevin and
wrwg
as code owners
⏱️ 4h 10m total CI duration on this PR
🚨 1 job on the last run was significantly faster/slower than expected
|
igor-aptos
force-pushed
the
igor/fungible_asset_improvement
branch
from
a75a3a7 to
06546cf
Compare
igor-aptos
added
CICD:run-execution-performance-test
igor-aptos
changed the title
| /// | ||
| /// Note: This function will abort on FAs with `derived_balance` hook set up. | ||
| /// Use `dispatchable_fungible_asset::balance` instead if you intend to work with those FAs. | ||
| public fun balance_snapshot<T: key>(store: Object<T>): AggregatorSnapshot<u64> acquires FungibleStore, ConcurrentFungibleBalance, DispatchFunctionStore { |
There was a problem hiding this comment.
why we need this function?
There was a problem hiding this comment.
The existing balance function already takes a snapshot right?
There was a problem hiding this comment.
So that it can be emitted to event?
There was a problem hiding this comment.
both balance and balance_snapshot will return the same value (irrespective of whether store is concurrent or not).
but if balance is concurrent, then balance_snapshot will not create a conflict - and callers can freely store it in another resource, or emit as an event, with keeping things parallel. In the future we will allow formulas to be applied to them as well.
| transaction_fee::burn_fee(gas_payer, burn_amount); | ||
| transaction_fee::burn_fee(gas_payer, burn_amount, account_addr != gas_payer); | ||
| } else if (transaction_fee_amount < storage_fee_refunded) { | ||
| let mint_amount = storage_fee_refunded - transaction_fee_amount; | ||
| transaction_fee::mint_and_refund(gas_payer, mint_amount); | ||
| transaction_fee::mint_and_refund(gas_payer, mint_amount, account_addr != gas_payer); |
There was a problem hiding this comment.
This would change any gas payer. So, it could change accounts that you don't necessarily want to change to concurrent.
This should be feature flagged.
There was a problem hiding this comment.
I don't think there needs to be a snapshot function, does the existing view function not work? If so, it needs to be made to work normally. Asking people to figure out which view function to make is a bad experience.
Force migrating people to concurrent balance should be feature gated or opt in. I fear this will break more than we expect
There was a problem hiding this comment.
In the current impl, we are automatically changing fee payer's account into a concurrent one right? From the fee payer account perspective I do share the same concern as Greg, yet I don't necessary see how a feature flag would change the situation here. However, I do think it's to the interest of everyone that gas fee payer should be using a concurrent balance store which could justify for the changes here.
The purpose of the feature flag is control when it goes through, so it doesn't roll through with other changes, though it's not reversible. |
|
should've given more context, sorry. replied inline for balance_snapshot usecase - of storing it in a resource/emitting an event, while keeping usage parallel. It is frameworks internal implementation detail which FungibleStore is concurrent and which is not - and it should be free to switch between them. I can add a feature flag to decouple framework release, from this change - but let me know if there are concerns about the actual change - of defaulting users (here fee-payers) to ConcurrentFungibleStore. In the future, if we improve ResourceGroups performance, we might switch everyone to ConcurrentFungibleStore. |
igor-aptos
force-pushed
the
igor/fungible_asset_improvement
branch
from
af00b05 to
c6dc8c9
Compare
|
I think it is better to use an opt-out model. If concurrent is default for gas payer and there are some people who do not this, we need another flag indicating that in the same RG. |
|
I've added the feature flag here |
Description
How Has This Been Tested?
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist