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

Crescendo #163

Closed
wants to merge 16 commits into from
Closed

Crescendo #163

wants to merge 16 commits into from

Conversation

austinkline
Copy link
Collaborator

@austinkline austinkline commented Apr 1, 2024

This PR covers changes needed to upgrade HybridCustody to Cadence 1.0

Some key points:

  1. Private Paths are not going to be in Cadence 1.0, so our standard path for getCapability that takes a CapabilityPath type had to be moved to take a Capability Controller ID instead.
  2. Because there are no private paths, we now have to use storage iteration for clearing out existing capabilities when things like ownership updates or sealing change.
  3. There are 4 entitlements added to the HybridCustody contract. Three of them map to a resource, and one of them is added just for publishing an account to a parent from a child.
  4. ResourceDestroyed events have been added to each core resource type. There was an issue with emitting some fields we might want in the event which might need some follow-up
  5. You can no longer get a public capability without so retrieving a public capability had to be moved to the CapabilityFactory interface.
  6. Since HybridCustody takes in a full AuthAccount reference currently, we have migrated all AuthAccount types to a fully entitled &Account reference. It does seem to need nearly all of them with the exception of the Contract entitlement.

The main questions coming out of this work as I see them are around Entitlements and whether the use of each entitlement gives us the access control pattern(s) we want. Personally, I had a hard time justifying splitting entitlements beyond each resource type.

- Switch dependency management to npm package for now since submodules aren't yet fully working
@austinkline austinkline changed the title [WIP] Crescendo Migration [WIP] Crescendo Apr 1, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2024

Codecov Report

Attention: Patch coverage is 80.72289% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 89.38%. Comparing base (84bb6f3) to head (53ff595).

Files Patch % Lines
contracts/HybridCustody.cdc 80.48% 16 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #163      +/-   ##
==========================================
+ Coverage   83.95%   89.38%   +5.43%     
==========================================
  Files           4        4              
  Lines         349      377      +28     
==========================================
+ Hits          293      337      +44     
+ Misses         56       40      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@austinkline austinkline changed the title [WIP] Crescendo Crescendo Apr 3, 2024
@austinkline austinkline closed this Apr 3, 2024
sisyphusSmiling added a commit that referenced this pull request Jun 27, 2024
Closes: #167 

While staging contracts on Mainnet, I realized the contract defined in
NFTCollectionFactory is names NFTProviderAndCollectionFactory which
collides with another contract. I assume this wasn't caught in #163 so
am submitting the fix in this PR.
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.

2 participants