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.

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

Merged
merged 7 commits into from
Feb 28, 2025

Conversation

blockiosaurus
Copy link
Contributor

Adding the ability to change the update authority or collection as the update delegate.

Copy link

vercel bot commented Dec 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mpl-core-js-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 28, 2025 8:38pm

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Benchmark

Benchmark suite Current: 8d8db08 Previous: f569292 Ratio
CU: create a new, empty asset 11206 Compute Units 11206 Compute Units 1
Space: create a new, empty asset 91 Bytes 91 Bytes 1
CU: create a new, empty asset with empty collection 22681 Compute Units 22681 Compute Units 1
Space: create a new, empty asset with empty collection 91 Bytes 91 Bytes 1
CU: create a new asset with plugins 35182 Compute Units 35182 Compute Units 1
Space: create a new asset with plugins 194 Bytes 194 Bytes 1
CU: create a new asset with plugins and empty collection 41202 Compute Units 41202 Compute Units 1
Space: create a new asset with plugins and empty collection 194 Bytes 194 Bytes 1
CU: list an asset 28101 Compute Units 28101 Compute Units 1
CU: sell an asset 44473 Compute Units 44473 Compute Units 1
CU: list an asset with empty collection 35663 Compute Units 35663 Compute Units 1
CU: sell an asset with empty collection 56825 Compute Units 56825 Compute Units 1
CU: list an asset with collection royalties 36434 Compute Units 36434 Compute Units 1
CU: sell an asset with collection royalties 61909 Compute Units 61909 Compute Units 1
CU: transfer an empty asset 6444 Compute Units 6444 Compute Units 1
CU: transfer an empty asset with empty collection 9043 Compute Units 9043 Compute Units 1
CU: transfer an asset with plugins 14769 Compute Units 14769 Compute Units 1
CU: transfer an asset with plugins and empty collection 17368 Compute Units 17368 Compute Units 1

This comment was automatically generated by workflow using github-action-benchmark.

nhanphan
nhanphan previously approved these changes Dec 30, 2024
@blockiosaurus
Copy link
Contributor Author

Huge thanks to @ASCorreia for his work on #217 regarding #208.

@danenbm danenbm self-requested a review February 27, 2025 23:35
Copy link
Contributor

@danenbm danenbm left a comment

Choose a reason for hiding this comment

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

Approved with comment to check

@blockiosaurus blockiosaurus merged commit 661d5e0 into main Feb 28, 2025
13 checks passed
@blockiosaurus blockiosaurus deleted the feat/update-delegate-ua-change branch February 28, 2025 20:56

Choose a reason for hiding this comment

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

PR Overview

This PR updates the update delegate logic to enable changing the update authority or collection by the update delegate while modifying client tests to verify the new behavior.

  • Modified conditional logic in the update delegate plugin to distinguish between asset and collection update rules.
  • Adjusted client tests (both in updateV2 and plugin tests) to assert state changes instead of expecting errors.
  • Updated the processor’s authority checks to ensure consistency with the new behavior.

Reviewed Changes

File Description
programs/mpl-core/src/plugins/internal/authority_managed/update_delegate.rs Updated update delegate plugin logic to handle asset and collection authority updates with revised conditionals.
clients/js/test/updateV2.test.ts Modified tests to assert asset state after update operations in line with new delegate behavior.
clients/js/test/plugins/collection/updateDelegate.test.ts Changed tests for collection update authority to reflect the new allowed behaviors.
clients/js/test/plugins/asset/updateDelegate.test.ts Revised tests for asset update authority updates via update delegate to assert state updates.
programs/mpl-core/src/processor/update.rs Adjusted authority check syntax in processor logic to align with updated delegate conditions.

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

programs/mpl-core/src/plugins/internal/authority_managed/update_delegate.rs:165

  • [nitpick] Consider adding explicit parentheses around the combined conditions to clarify the intended logical grouping and improve readability.
if (ctx.resolved_authorities.is_some() && ctx.resolved_authorities.unwrap().contains(ctx.self_authority)

programs/mpl-core/src/plugins/internal/authority_managed/update_delegate.rs:182

  • [nitpick] Consider caching the results of unwrap() calls for ctx.new_asset_authority and ctx.collection_info to enhance readability and reduce repetitive calls.
&& ctx.new_asset_authority.unwrap() != &UpdateAuthority::Collection(*ctx.collection_info.unwrap().key)

programs/mpl-core/src/processor/update.rs:226

  • [nitpick] Ensure that using authority.key without the '&' is consistent with the type requirements of additional_delegates.contains; adding a comment to document this intentional change could help future maintainers.
&& !plugin.additional_delegates.contains(authority.key)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants