[framework] implement immovable objects #13175
Conversation
⏱️ 2h 12m total CI duration on this PR
🚨 1 job on the last run was significantly faster/slower than expected
|
davidiw
requested review from
lightmark,
junkil-park,
wrwg,
JohnChangUK,
kshitjee,
movekevin,
gregnazario and
igor-aptos
and removed request for
junkil-park,
wrwg and
movekevin
|
There's object transfer and fa transfer, nomenclature is hard 😬 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13175 +/- ##
========================================
Coverage 57.5% 57.6%
========================================
Files 833 834 +1
Lines 198121 198399 +278
========================================
+ Hits 113999 114334 +335
+ Misses 84122 84065 -57 ☔ View full report in Codecov by Sentry. |
|
Currently, if you first call to enable ungated transfer, can to make it immovable doesn't do anything. Should we move the check to one of transfer implementation functions? "Move" is also overloaded a lot. Can we just be more explicit? CannotChangeOwner, or ImmutableOwner, or something like that? |
Did you type this on a phone :P I might be missing your point, this locks all possible transfers of the object after it has been set immovable. I'm not sure there's a logical error, we can always surface different errors and have more checks if we want to be more pedantic. I'll update the AIP and think about the name... |
| } | ||
|
|
||
| /// Returns true if the FA is untransferable. | ||
| public fun is_immovable<T: key>(metadata: Object<T>): bool { |
There was a problem hiding this comment.
Worth adding #[view] above?
| @@ -234,6 +240,19 @@ module aptos_framework::fungible_asset { | |||
| object::object_from_constructor_ref<Metadata>(constructor_ref) | |||
| } | |||
|
|
|||
| /// Set that only untransferable stores can be create for this fungible asset. | |||
There was a problem hiding this comment.
| /// Set that only untransferable stores can be create for this fungible asset. | |
| /// Set that only untransferable stores can be created for this fungible asset. |
| @@ -120,6 +122,10 @@ module aptos_framework::fungible_asset { | |||
| project_uri: String, | |||
| } | |||
|
|
|||
| #[resource_group_member(group = aptos_framework::object::ObjectGroup)] | |||
| /// Defnes a fungible asset stores as being untransferable at the object layer. | |||
There was a problem hiding this comment.
| /// Defnes a fungible asset stores as being untransferable at the object layer. | |
| /// Defines a fungible asset store as being untransferable at the object layer. |
| @@ -120,6 +122,10 @@ module aptos_framework::fungible_asset { | |||
| project_uri: String, | |||
| } | |||
|
|
|||
| #[resource_group_member(group = aptos_framework::object::ObjectGroup)] | |||
| /// Defines a fungible asset stores as being untransferable at the object layer. | |||
There was a problem hiding this comment.
It took me a minute to understand this is on FungibleAsset , saying that all of their FungibleStore's are immutable.
| /// Defines a fungible asset stores as being untransferable at the object layer. | |
| /// Defines a FungibleAsset, such that all FungibleStores are untransferable (marked at such at the object layer). | |
| /// All primary fungible stores are always untransferable (i.e. even without this). This applies to the secondary stores. |
|
|
||
| #[view] | ||
| /// Returns true if the FA is untransferable. | ||
| public fun is_untransferable<T: key>(metadata: Object<T>): bool { |
There was a problem hiding this comment.
T can only be Metadata, right? why not make (metadata: Object<Metadata>)
| @@ -120,6 +122,10 @@ module aptos_framework::fungible_asset { | |||
| project_uri: String, | |||
| } | |||
|
|
|||
| #[resource_group_member(group = aptos_framework::object::ObjectGroup)] | |||
| /// Defines a fungible asset stores as being untransferable at the object layer. | |||
| struct Untransferable has key {} | |||
There was a problem hiding this comment.
Can we use an enum style with another struct Transferrable mutually exclusive with Untransferable so TransferRef and Untransferrable object are also mutually exclusive?
if Untransferable exists, no TransferRef will be generated.
if TransferRef is generated, Transferable will exists and you cannot make an object Untransferrable. This way we don't have to exists<Transferrable> in the following function calls because the enable_ungated_transfer is always false and we are ensured TransferRef does not exist for this object.
There was a problem hiding this comment.
I think this may be cleaner.
There was a problem hiding this comment.
That doesn't work, since either can be generated if given a ConstructorRef prior to calling set_untransferrable.
There was a problem hiding this comment.
whoever gets called first will set a flag there which prohibits the other. So it should work
Make it so that objects can be made untransferable via the constructor ref. Even if other entities have access to the constructor ref. Add the functionality to fungible asset to make untransferable stores by default for assets that opt into the behavior. Also offer a root_owner function to determine the deepest node that owns a nested object.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
areshand
added
the
CICD:run-execution-performance-test
Make it so that objects can be made immovable via the constructor ref. Even if other entities have access to the constructor ref.
Add the functionality to fungible asset to make immovable stores by default for assets that opt into the behavior.
Also offer a root_owner function to determine the deepest node that owns a nested object.
See aptos-foundation/AIPs#414
Description
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Lots of unit tests
Key Areas to Review
There's really not that much code...
Checklist