Crescendo #164

austinkline · 2024-04-03T19:22:57Z

This PR covers changes needed to upgrade HybridCustody to Cadence 1.0

Some key points:

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.
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.
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.
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
You can no longer get a public capability without so retrieving a public capability had to be moved to the CapabilityFactory interface.
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

…and give borrowAccount the Owner entitlement

joshuahannan

Just reviewed the contracts so far. They look pretty good! I made some comments about migrating capability types, but I'm not 100% sure about the answer, so I didn't put it on every single instance that the comment would apply to in case I'm wrong. I'll flag the other ones if it turns out I am right

contracts/HybridCustody.cdc

contracts/CapabilityDelegator.cdc

contracts/CapabilityFactory.cdc

contracts/CapabilityDelegator.cdc

bjartek · 2024-04-03T21:36:25Z

contracts/HybridCustody.cdc

@@ -27,39 +29,43 @@ import "CapabilityFilter"
 ///
 /// Repo reference: https://github.com/onflow/hybrid-custody
 ///
-pub contract HybridCustody {
+access(all) contract HybridCustody {
+    access(all) entitlement Owner


I think adding some notes about entitlements that explains what permission they will give is good here. Like will Owner give you all of the below or not? I think best practise in flow around entitlements should describe what they do briefly where they are defined. thoughts?

Not opposed at all. I started writing some comments out for each entitlement then realized they might just end up changing anyway which is why they're missing right now

contracts/HybridCustody.cdc

sisyphusSmiling

Great refactor so far, thank you! I was only able to look over the contracts, but left some comments. I'm also wondering if we'll need to introduce new Factory implementations for deployment covering Vault & Collection with the migration given the move from nested resources to interfaces within FT & NFT v2 standards.

contracts/CapabilityFactory.cdc

contracts/CapabilityFilter.cdc

contracts/factories/FTAllFactory.cdc

contracts/HybridCustody.cdc

austinkline · 2024-04-12T14:59:06Z

Quick update to this PR that I'm back from a company offsite so we're picking this up again.

Core questions are around entitlements (expected, I'll start focusing on them soon), and on how migrations are going to work.

For migrations, my understanding right now is that all capabilities are going to be migrated to their conformance types. That is Capability<&T{I}> will be migrated to Capability<&{I}> in all cases. So if we want things to migrate successfully, it is the right thing to go with Capability<&{I}> versus Capability<&T>. I will operate on this assumption and then ask Bastian or someone else on the cadence team to review what they can before we finalize all of this

codecov-commenter · 2024-04-12T15:05:59Z

Codecov Report

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

Project coverage is 91.48%. Comparing base (84bb6f3) to head (756be5f).

Files	Patch %	Lines
contracts/HybridCustody.cdc	89.61%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #164      +/-   ##
==========================================
+ Coverage   83.95%   91.48%   +7.52%     
==========================================
  Files           4        4              
  Lines         349      364      +15     
==========================================
+ Hits          293      333      +40     
+ Misses         56       31      -25

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

sisyphusSmiling

Only got through the contracts again but can revisit tomorrow to cover txns & scripts. The entitlement assignments make sense to me, they look well done! Just have one question about GetterPrivate using Get vs Capabilities - curious what you think there.

contracts/HybridCustody.cdc

contracts/standard/ExampleNFT.cdc

contracts/standard/ExampleToken.cdc

contracts/CapabilityDelegator.cdc

contracts/CapabilityFactory.cdc

contracts/HybridCustody.cdc

…e Owner entitlement

sisyphusSmiling

Great work!

Update: All HC contracts (except FTVaultFactory and NFTCollectionFactory) up to 2b22935 on crescendo branch have been staged on Testnet. However, as-is I needed to skip validation due to errors. See ongoing discussion around migration errors in this discord thread.

sisyphusSmiling · 2024-05-02T16:05:39Z

Update after latest migration - looks like migration failed. A couple failures stand out:

References to NonFungibleToken.Owner entitlement caused failures in NFTProviderFactory and NFTProviderAndCollectionFactory
Disjunction access modifiers in CapabiltyFactory, CapabiltyFilter and CapabiltyDelegator resulted in unsafe entitlements migration.
Capabilities no longer return optional values which resulted in failures within HybridCustody, but I'm sure there are occurrences elsewhere throughout transactions/scripts where capability retrieval is optionally chained
OwnedAccount.removeParent is access(Owner | Remove) while OwnedAccountPrivate interface modifier is simply Owner

I think addressing these should fix the migration issue. We'll need a new pre-release version of CLI (currently in progress) to validate the changes via Cadence tests though since the capability retrieval pattern has been updated since the current v1.18.0-cadence-v1.0.0-preview.22

austinkline · 2024-05-06T14:03:21Z

Update after latest migration - looks like migration failed. A couple failures stand out:

References to NonFungibleToken.Owner entitlement caused failures in NFTProviderFactory and NFTProviderAndCollectionFactory

Disjunction access modifiers in CapabiltyFactory, CapabiltyFilter and CapabiltyDelegator resulted in unsafe entitlements migration.

Capabilities no longer return optional values which resulted in failures within HybridCustody, but I'm sure there are occurrences elsewhere throughout transactions/scripts where capability retrieval is optionally chained

OwnedAccount.removeParent is access(Owner | Remove) while OwnedAccountPrivate interface modifier is simply Owner

I think addressing these should fix the migration issue. We'll need a new pre-release version of CLI (currently in progress) to validate the changes via Cadence tests though since the capability retrieval pattern has been updated since the current v1.18.0-cadence-v1.0.0-preview.22

Thanks for summarizing 🙏

I believe I've addressed everything for this week's migration, but if we're able to dry-run it locally that's probably a good idea as well

contracts/HybridCustody.cdc

sisyphusSmiling · 2024-05-08T02:30:16Z

Local migration passed! All contracts up to this point have been staged on Testnet, so we'll get a sanity check in the next network migration run.

sisyphusSmiling · 2024-05-16T18:56:51Z

Following up on office hours discussion about merging these changes. Looks like the past two migrations have been successful. Maybe we a last call for reviews before merging - thoughts @austinkline?

austinkline · 2024-05-16T21:02:56Z

Following up on office hours discussion about merging these changes. Looks like the past two migrations have been successful. Maybe we a last call for reviews before merging - thoughts @austinkline?

Good with me! I'll reach out to folks and see if we can't get some approvals/comments going

austinkline · 2024-05-28T20:23:06Z

Merging! Thanks everyone for the feedback and participation in this PR. We still have time until Crescendo goes live, so if there are any other things that come up or need addressing, please open a ticket and flag it 🙏

https://discord.com/channels/613813861610684416/1162086721471647874/1245041945013780601

Closes: #165 #171 ### Description - Replaces existing node modules dependency system with Flow CLI's dependency manager for consistency with other onflow repos - Updates CI commands to use the Flow CLI `flow` command found in `v2.0.0` - Removes Previewnet deployment addresses as that network will be deprecated - Removes the code normalization script which has been integrated with the `flow test` command - Removes the outdated `.gitmodules` file, a remnant from when this repo used git submodules Also, outside of the context of this PR, the new `NFTCollectionFactory` and `FTVaultFactory` (#164) contracts have been deployed on Testnet & Mainnet. After deployment, the existing hosted factory managers (see README) have been configured with the setup transactions already found in this repo.

austinkline added 16 commits March 23, 2024 16:02

- Update all HybridCustody contract dependencies

82d8da8

- Switch dependency management to npm package for now since submodules aren't yet fully working

address all contract errors/warnings

acaa263

split entitlements up

dd03073

remove placeholder entitlement

ec07d6d

move public capability retrieval to the CapabilityFactory

2bff3f3

fix CapabilityDelegator tests

58b081f

fix CapabilityFactory tests

42746d4

fix HybridCustody tests

1301040

configure tests with flow-c1

1731fe6

add exclusions to code coverage

317016a

add Burnable Interface

ed12538

fix access level for burn callback

b075ab0

add missing tests, make a new _borrowAccount method for internal use …

96fbbbf

…and give borrowAccount the Owner entitlement

add tests for burn callback

53ff595

add test for public capabilities

2d12790

add comments to burnCallback methods

0336192

joshuahannan reviewed Apr 3, 2024

View reviewed changes

bjartek suggested changes Apr 3, 2024

View reviewed changes

sisyphusSmiling reviewed Apr 4, 2024

View reviewed changes

remove all variables and references to private paths

da10712

austinkline added 4 commits April 12, 2024 09:02

update entitlements

419deea

more entitlements changes

ce76340

add {Collection} and {Vault} factories

3e923b5

add missing contract in config

83b7571

sisyphusSmiling reviewed Apr 18, 2024

View reviewed changes

contracts/HybridCustody.cdc Show resolved Hide resolved

contracts/HybridCustody.cdc Outdated Show resolved Hide resolved

remove incorrect entitlement usage

0c8ef75

bjartek reviewed Apr 22, 2024

View reviewed changes

bjartek approved these changes Apr 22, 2024

View reviewed changes

bluesign reviewed Apr 24, 2024

View reviewed changes

contracts/CapabilityFactory.cdc Show resolved Hide resolved

btspoony reviewed Apr 24, 2024

View reviewed changes

contracts/HybridCustody.cdc Show resolved Hide resolved

austinkline added 2 commits April 24, 2024 09:18

remove built-in entitlements

547abbe

correct entitlements for NFT.Withdraw because migrations also need th…

2b22935

…e Owner entitlement

sisyphusSmiling mentioned this pull request Apr 24, 2024

Deploy NFTCollectionFactory and FTVaultFactory contracts post-migration #165

Closed

sisyphusSmiling approved these changes Apr 24, 2024

View reviewed changes

update Burner testnet alias

0c0243b

sync new nft standard and public capability retrieval changes

1346ae1

add temporary local migration report

6b46b3b

sisyphusSmiling reviewed May 7, 2024

View reviewed changes

contracts/HybridCustody.cdc Outdated Show resolved Hide resolved

austinkline and others added 7 commits May 7, 2024 16:05

remove all conjunctions from function acess modifiers

8ae4eee

update local migration report

2671d5d

fix FTVaultFactory

b646f85

fix migration types for child account factory and delegator

051c49c

fix tests

7562ab1

update ChildAccount.delegator field to CapabilityDelegator.Delegator

0b41ec7

remove temp report directory

756be5f

bjartek approved these changes May 17, 2024

View reviewed changes

austinkline merged commit 1a4224c into main May 28, 2024
1 check passed

austinkline deleted the crescendo branch May 28, 2024 20:25

sisyphusSmiling mentioned this pull request Jul 25, 2024

[DEPLOYMENT] Deploy to Previewnet and update deployment details #169

Closed

This was referenced Sep 5, 2024

Update HybridCustody repo for Cadence 1.0 #158

Closed

[FEATURE] Migrate to Capability Controllers #120

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crescendo #164

Crescendo #164

austinkline commented Apr 3, 2024

joshuahannan left a comment

bjartek Apr 3, 2024

austinkline Apr 4, 2024

sisyphusSmiling left a comment

austinkline commented Apr 12, 2024

codecov-commenter commented Apr 12, 2024 •

edited

Loading

sisyphusSmiling left a comment

sisyphusSmiling left a comment •

edited

Loading

sisyphusSmiling commented May 2, 2024 •

edited

Loading

austinkline commented May 6, 2024

sisyphusSmiling commented May 8, 2024

sisyphusSmiling commented May 16, 2024

austinkline commented May 16, 2024

austinkline commented May 28, 2024

Crescendo #164

Crescendo #164

Conversation

austinkline commented Apr 3, 2024

joshuahannan left a comment

Choose a reason for hiding this comment

bjartek Apr 3, 2024

Choose a reason for hiding this comment

austinkline Apr 4, 2024

Choose a reason for hiding this comment

sisyphusSmiling left a comment

Choose a reason for hiding this comment

austinkline commented Apr 12, 2024

codecov-commenter commented Apr 12, 2024 • edited Loading

Codecov Report

sisyphusSmiling left a comment

Choose a reason for hiding this comment

sisyphusSmiling left a comment • edited Loading

Choose a reason for hiding this comment

sisyphusSmiling commented May 2, 2024 • edited Loading

austinkline commented May 6, 2024

sisyphusSmiling commented May 8, 2024

sisyphusSmiling commented May 16, 2024

austinkline commented May 16, 2024

austinkline commented May 28, 2024

codecov-commenter commented Apr 12, 2024 •

edited

Loading

sisyphusSmiling left a comment •

edited

Loading

sisyphusSmiling commented May 2, 2024 •

edited

Loading