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.

Sign up for GitHub

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

UpdateDelegate Change UA/Collection #204

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

UpdateDelegate Change UA/Collection #204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions clients/js/test/plugins/asset/updateDelegate.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,7 +689,7 @@ test('it cannot add updateDelegate plugin with additional delegate as additional
await t.throwsAsync(result, { name: 'NoApprovals' });
});

test('it cannot update the update authority of the asset as an updateDelegate additional delegate', async (t) => {
test('it can update the update authority of the asset as an updateDelegate additional delegate', async (t) => {
const umi = await createUmi();
const updateDelegate = generateSigner(umi);
const updateDelegate2 = generateSigner(umi);
Expand All @@ -703,16 +703,21 @@ test('it cannot update the update authority of the asset as an updateDelegate ad
],
});

const result = update(umi, {
await update(umi, {
asset,
authority: updateDelegate,
newUpdateAuthority: updateAuthority('Address', [updateDelegate2.publicKey]),
}).sendAndConfirm(umi);

await t.throwsAsync(result, { name: 'NoApprovals' });
await assertAsset(t, umi, {
...DEFAULT_ASSET,
asset: asset.publicKey,
owner: umi.identity.publicKey,
updateAuthority: { type: 'Address', address: updateDelegate2.publicKey },
});
});

test('it cannot update the update authority of the asset as an updateDelegate root authority', async (t) => {
test('it can update the update authority of the asset as an updateDelegate root authority', async (t) => {
const umi = await createUmi();
const updateDelegate = generateSigner(umi);
const updateDelegate2 = generateSigner(umi);
Expand All @@ -727,13 +732,18 @@ test('it cannot update the update authority of the asset as an updateDelegate ro
],
});

const result = update(umi, {
await update(umi, {
asset,
authority: updateDelegate,
newUpdateAuthority: updateAuthority('Address', [updateDelegate2.publicKey]),
}).sendAndConfirm(umi);

await t.throwsAsync(result, { name: 'NoApprovals' });
await assertAsset(t, umi, {
...DEFAULT_ASSET,
asset: asset.publicKey,
owner: umi.identity.publicKey,
updateAuthority: { type: 'Address', address: updateDelegate2.publicKey },
});
});

test('an updateDelegate can add a plugin to an asset', async (t) => {
Expand Down
20 changes: 14 additions & 6 deletions clients/js/test/plugins/collection/updateDelegate.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,7 +654,7 @@ test('it can approve/revoke non-updateDelegate plugin on an asset as collection
});
});

test('it cannot update the update authority of the collection as an updateDelegate additional delegate', async (t) => {
test('it can update the update authority of the collection as an updateDelegate additional delegate', async (t) => {
const umi = await createUmi();
const updateDelegate = generateSigner(umi);
const updateDelegate2 = generateSigner(umi);
Expand All @@ -668,16 +668,20 @@ test('it cannot update the update authority of the collection as an updateDelega
],
});

const result = updateCollection(umi, {
await updateCollection(umi, {
collection: collection.publicKey,
authority: updateDelegate,
newUpdateAuthority: updateDelegate2.publicKey,
}).sendAndConfirm(umi);

await t.throwsAsync(result, { name: 'InvalidAuthority' });
await assertCollection(t, umi, {
...DEFAULT_COLLECTION,
collection: collection.publicKey,
updateAuthority: updateDelegate2.publicKey,
});
});

test('it cannot update the update authority of the collection as an updateDelegate root authority', async (t) => {
test('it can update the update authority of the collection as an updateDelegate root authority', async (t) => {
const umi = await createUmi();
const updateDelegate = generateSigner(umi);
const updateDelegate2 = generateSigner(umi);
Expand All @@ -692,13 +696,17 @@ test('it cannot update the update authority of the collection as an updateDelega
],
});

const result = updateCollection(umi, {
await updateCollection(umi, {
collection: collection.publicKey,
authority: updateDelegate,
newUpdateAuthority: updateDelegate2.publicKey,
}).sendAndConfirm(umi);

await t.throwsAsync(result, { name: 'InvalidAuthority' });
await assertCollection(t, umi, {
...DEFAULT_COLLECTION,
collection: collection.publicKey,
updateAuthority: updateDelegate2.publicKey,
});
});

test('it can update collection details as an updateDelegate additional delegate', async (t) => {
Expand Down
22 changes: 16 additions & 6 deletions clients/js/test/updateV2.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1046,7 +1046,7 @@ test('it can change an asset collection using same update authority (delegate ex
});
});

test('it cannot remove an asset from collection using update delegate', async (t) => {
test('it can remove an asset from collection using update delegate', async (t) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If my other comment is true then I think we should keep tests for not allowing asset update delegates and asset additional update delegates.

const umi = await createUmi();
const collectionAuthority = generateSigner(umi);
const { asset, collection } = await createAssetWithCollection(
Expand Down Expand Up @@ -1107,17 +1107,22 @@ test('it cannot remove an asset from collection using update delegate', async (t
numMinted: 1,
});

const result = update(umi, {
await update(umi, {
asset,
collection,
newUpdateAuthority: updateAuthority('Address', [umi.identity.publicKey]),
authority: updateDelegate,
}).sendAndConfirm(umi);

await t.throwsAsync(result, { name: 'NoApprovals' });
await assertAsset(t, umi, {
...DEFAULT_ASSET,
asset: asset.publicKey,
owner: umi.identity.publicKey,
updateAuthority: { type: 'Address', address: umi.identity.publicKey },
});
});

test('it cannot remove an asset from collection using additional update delegate', async (t) => {
test('it can remove an asset from collection using additional update delegate', async (t) => {
const umi = await createUmi();
const collectionAuthority = generateSigner(umi);
const { asset, collection } = await createAssetWithCollection(
Expand Down Expand Up @@ -1157,14 +1162,19 @@ test('it cannot remove an asset from collection using additional update delegate
numMinted: 1,
});

const result = update(umi, {
await update(umi, {
asset,
collection,
newUpdateAuthority: updateAuthority('Address', [umi.identity.publicKey]),
authority: additionalDelegate,
}).sendAndConfirm(umi);

await t.throwsAsync(result, { name: 'NoApprovals' });
await assertAsset(t, umi, {
...DEFAULT_ASSET,
asset: asset.publicKey,
owner: umi.identity.publicKey,
updateAuthority: { type: 'Address', address: umi.identity.publicKey },
});
});

test('it cannot add asset to collection using additional update delegate on new collection', async (t) => {
Expand Down
14 changes: 6 additions & 8 deletions programs/mpl-core/src/plugins/internal/authority_managed/update_delegate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,14 +164,12 @@ impl PluginValidation for UpdateDelegate {
&self,
ctx: &PluginValidationContext,
) -> Result<ValidationResult, ProgramError> {
if ((ctx.resolved_authorities.is_some()
&& ctx
.resolved_authorities
.unwrap()
.contains(ctx.self_authority))
|| self.additional_delegates.contains(ctx.authority_info.key))
// We do not allow the root authority (either Collection or Address) to be changed by this delegate.
&& ctx.new_collection_authority.is_none() && ctx.new_asset_authority.is_none()
if (ctx.resolved_authorities.is_some()
&& ctx
.resolved_authorities
.unwrap()
.contains(ctx.self_authority))
|| self.additional_delegates.contains(ctx.authority_info.key)
Comment on lines -167 to +172
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 validate_update is used for both asset plugins and collection plugins. So wouldn't this allow an update delegate on an asset to remove itself from a collection, which I think we don't want per the offline discussion?

{
approve!()
} else {
Expand Down
Loading