allow migrate object from one protocol to another#9
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds capability migration functionality to the protocol object framework, enabling the transfer of object management from one protocol to another. This is useful for protocol upgrades or governance changes where ownership of objects needs to be transferred between protocol implementations.
Key Changes:
- Added
migrateandmigrate_allfunctions to bothprotocol_objectandprotocol_fungible_assetmodules - Migration works by moving
Capabilityresources fromOldProtocoltoNewProtocoltype parameters - The
migrate_allfunctions handle migrating all standard capabilities in a single call, withExtendRefmigrated last inprotocol_objectto maintain authorization throughout the process
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| sources/protocol_object.move | Adds migrate<OldProtocol, NewProtocol, R> and migrate_all<OldProtocol, NewProtocol> functions to migrate protocol object capabilities (ExtendRef, DeriveRef, DeleteRef, TransferRef) between protocols |
| sources/protocol_fungible_asset.move | Adds migrate<OldProtocol, NewProtocol, R> and migrate_all<OldProtocol, NewProtocol> functions to migrate fungible asset capabilities (MintRef, BurnRef, TransferRef, MutateMetadataRef, RawSupplyRef, RawBalanceRef) between protocols |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| /// Migrates a specific capability `R` from `OldProtocol` to `NewProtocol`. | ||
| /// This allows transferring the management of an object's capability to a new protocol. |
There was a problem hiding this comment.
The documentation should clarify the requirements for migration. Specifically:
- The underlying fungible asset object must have
ExtendRefdelegated toOldProtocol(viaprotocol_objectmodule) - The function uses the object's
ExtendRefto authorize moving capabilities
Suggested addition:
/// Migrates a specific capability `R` from `OldProtocol` to `NewProtocol`.
/// This allows transferring the management of an object's capability to a new protocol.
///
/// **Requirements:**
/// - The underlying object must have `ExtendRef` delegated to `OldProtocol` (via protocol_object module)| /// This allows transferring the management of an object's capability to a new protocol. | |
| /// This allows transferring the management of an object's capability to a new protocol. | |
| /// | |
| /// # Requirements | |
| /// - The underlying fungible asset object must have `ExtendRef` delegated to `OldProtocol` (via `protocol_object` module). | |
| /// - The function uses the object's `ExtendRef` to authorize moving capabilities. |
| migrate<OldProtocol, NewProtocol, DeriveRef>(old_protocol, object_address); | ||
| migrate<OldProtocol, NewProtocol, TransferRef>(old_protocol, object_address); | ||
| migrate<OldProtocol, NewProtocol, DeleteRef>(old_protocol, object_address); | ||
| // Migrate ExtendRef last because it is needed for authorization (via object_signer) |
There was a problem hiding this comment.
While the comment correctly notes that ExtendRef is needed for authorization, it doesn't explain the full picture. Each individual call to migrate (lines 233-237) internally calls object_signer which requires ExtendRef to exist. Therefore, ExtendRef must be present throughout ALL migrations, not just when migrating ExtendRef itself.
Consider enhancing the comment:
// Migrate ExtendRef last because each migrate() call requires ExtendRef for authorization.
// Migrating ExtendRef earlier would make it unavailable for subsequent migrations.| // Migrate ExtendRef last because it is needed for authorization (via object_signer) | |
| // Migrate ExtendRef last because each migrate() call requires ExtendRef for authorization. | |
| // Migrating ExtendRef earlier would make it unavailable for subsequent migrations. |
| } | ||
|
|
||
| /// Migrates all standard protocol object capabilities (`ExtendRef`, `DeriveRef`, `DeleteRef`, `TransferRef`) | ||
| /// from `OldProtocol` to `NewProtocol`. |
There was a problem hiding this comment.
The documentation should clarify the expected state during migration. Specifically:
- All capabilities should be in storage (not currently extracted via
extract_ref) before migration - If a capability is currently extracted, it will be silently skipped during migration
Consider adding to the documentation:
/// Migrates all standard protocol object capabilities (`ExtendRef`, `DeriveRef`, `DeleteRef`, `TransferRef`)
/// from `OldProtocol` to `NewProtocol`.
///
/// **Requirements:**
/// - All capabilities should be in storage (not extracted) before calling this function
/// - Capabilities that don't exist or are currently extracted will be silently skipped| /// from `OldProtocol` to `NewProtocol`. | |
| /// from `OldProtocol` to `NewProtocol`. | |
| /// | |
| /// **Requirements:** | |
| /// - All capabilities should be in storage (not extracted) before calling this function | |
| /// - Capabilities that don't exist or are currently extracted will be silently skipped |
| let object_signer = object_signer(old_protocol, object_address); | ||
| if (exists<Capability<OldProtocol, R>>(object_address)) { |
There was a problem hiding this comment.
The object_signer call at line 220 will abort if Capability<OldProtocol, ExtendRef> doesn't exist at object_address, even when trying to migrate a different capability R. This breaks the function's silent no-op behavior when the capability doesn't exist (line 221).
The object_signer call should be moved inside the if block after confirming the capability exists:
public fun migrate<OldProtocol, NewProtocol, R: store>(
old_protocol: &OldProtocol,
object_address: address
) acquires Capability {
if (exists<Capability<OldProtocol, R>>(object_address)) {
let object_signer = object_signer(old_protocol, object_address);
let Capability { inner } = move_from<Capability<OldProtocol, R>>(object_address);
move_to(&object_signer, Capability<NewProtocol, R> { inner });
}
}Note: This still requires ExtendRef to be present under OldProtocol for the migration to work, which should be documented.
| let object_signer = object_signer(old_protocol, object_address); | |
| if (exists<Capability<OldProtocol, R>>(object_address)) { | |
| if (exists<Capability<OldProtocol, R>>(object_address)) { | |
| let object_signer = object_signer(old_protocol, object_address); |
| } | ||
| } | ||
|
|
||
| /// Migrates all standard fungible asset capabilities from `OldProtocol` to `NewProtocol`. |
There was a problem hiding this comment.
The documentation should clarify the expected state during migration. Consider adding:
/// Migrates all standard fungible asset capabilities from `OldProtocol` to `NewProtocol`.
///
/// **Requirements:**
/// - All capabilities should be in storage (not extracted) before calling this function
/// - Capabilities that don't exist or are currently extracted will be silently skipped
/// - The underlying object must have `ExtendRef` delegated to `OldProtocol`| /// Migrates all standard fungible asset capabilities from `OldProtocol` to `NewProtocol`. | |
| /// Migrates all standard fungible asset capabilities from `OldProtocol` to `NewProtocol`. | |
| /// | |
| /// **Requirements:** | |
| /// - All capabilities should be in storage (not extracted) before calling this function | |
| /// - Capabilities that don't exist or are currently extracted will be silently skipped | |
| /// - The underlying object must have `ExtendRef` delegated to `OldProtocol` |
| let object_signer = protocol_object::protocol_object::object_signer(old_protocol, object_address); | ||
| if (exists<Capability<OldProtocol, R>>(object_address)) { |
There was a problem hiding this comment.
The object_signer call at line 490 will abort if Capability<OldProtocol, ExtendRef> doesn't exist at object_address, even when trying to migrate a different capability R. This breaks the function's silent no-op behavior when the capability doesn't exist (line 491).
The object_signer call should be moved inside the if block after confirming the capability exists:
public fun migrate<OldProtocol, NewProtocol, R: store>(
old_protocol: &OldProtocol,
object_address: address
) acquires Capability {
if (exists<Capability<OldProtocol, R>>(object_address)) {
let object_signer = protocol_object::protocol_object::object_signer(old_protocol, object_address);
let Capability { inner } = move_from<Capability<OldProtocol, R>>(object_address);
move_to(&object_signer, Capability<NewProtocol, R> { inner });
}
}Note: This still requires the underlying object to have ExtendRef delegated to OldProtocol for the migration to work, which should be documented.
| let object_signer = protocol_object::protocol_object::object_signer(old_protocol, object_address); | |
| if (exists<Capability<OldProtocol, R>>(object_address)) { | |
| if (exists<Capability<OldProtocol, R>>(object_address)) { | |
| let object_signer = protocol_object::protocol_object::object_signer(old_protocol, object_address); |
| /// Note: If `R` is `ExtendRef`, this should be the last capability migrated, as `ExtendRef` | ||
| /// is required to generate the object signer needed for migration. |
There was a problem hiding this comment.
The documentation for the migrate function should clarify that ExtendRef must be delegated to OldProtocol for this function to work, regardless of which capability R is being migrated. The current note only mentions this constraint when R is ExtendRef, but the object_signer call requires ExtendRef to be present for all migrations.
Suggested improvement:
/// Migrates a specific capability `R` from `OldProtocol` to `NewProtocol`.
/// This allows transferring the management of an object's capability to a new protocol.
///
/// **Requirements:**
/// - `ExtendRef` must be delegated to `OldProtocol` to authorize the migration.
/// - If migrating `ExtendRef` itself, it should be the last capability migrated.| /// Note: If `R` is `ExtendRef`, this should be the last capability migrated, as `ExtendRef` | |
| /// is required to generate the object signer needed for migration. | |
| /// | |
| /// **Requirements:** | |
| /// - `ExtendRef` must be delegated to `OldProtocol` to authorize the migration, regardless of which capability `R` is being migrated. | |
| /// - If migrating `ExtendRef` itself, it should be the last capability migrated, as `ExtendRef` is required to generate the object signer needed for migration. |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@yeap-oneke I've opened a new pull request, #10, to work on those changes. Once the pull request is ready, I'll request review from you. |
No description provided.