Infer the missing borrow type for storage capabilities #6314
Conversation
| @@ -345,7 +347,71 @@ func (m *IssueStorageCapConMigration) Close() error { | |||
| return nil | |||
| } | |||
|
|
|||
| var _ AccountBasedMigration = &IssueStorageCapConMigration{} | |||
| func (m *IssueStorageCapConMigration) issueAccountCapabilities( | |||
There was a problem hiding this comment.
This method was copied over and modified from Cadence side. Need to access the storage, etc. so having it here makes more sense.
The old method can be removed from cadence (tech-debt).
There was a problem hiding this comment.
We might need to re-incorporate the logic from onflow/cadence#3516 once it's merged, or move the changes here to Cadence.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6314 +/- ##
==========================================
+ Coverage 41.50% 41.51% +0.01%
==========================================
Files 1965 2009 +44
Lines 140713 143244 +2531
==========================================
+ Hits 58400 59473 +1073
- Misses 76236 77587 +1351
- Partials 6077 6184 +107
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
9f9f6d3 to
f28b494
Compare
|
Might not work after onflow/cadence#3516. Need to re-visit. |
There was a problem hiding this comment.
Logic and tests look good!
As discussed in the Slack thread, unfortunately due to onflow/cadence#3516, we probably need to refactor the improvement here once that PR is in
| @@ -345,7 +347,71 @@ func (m *IssueStorageCapConMigration) Close() error { | |||
| return nil | |||
| } | |||
|
|
|||
| var _ AccountBasedMigration = &IssueStorageCapConMigration{} | |||
| func (m *IssueStorageCapConMigration) issueAccountCapabilities( | |||
There was a problem hiding this comment.
We might need to re-incorporate the logic from onflow/cadence#3516 once it's merged, or move the changes here to Cadence.
| // If the borrow type is missing, treat it as `AnyStruct` or `AnyResource`. | ||
| // To determine whether it is a resource, read the target path. |
There was a problem hiding this comment.
The capability has direct access to the storage path, without any borrow type "restriction", so theoretically the capability could be borrowed with any type. Inferring to &AnyStruct / &AnyResource (or even &Any) might not be sufficient, for two reasons:
-
The capability could be borrowed with entitlements, but the inferred type does not grant any.
For example, imagine a capability targeting
/storage/flowTokenVault, not having a borrow type, and getting borrowed withauth(FungibleToken.Withdraw) &FlowToken.Vault. -
The value stored under the path could change (e.g. even from a resource to a struct or vice-versa), making the inferred borrow type "too restrictive"
The first issue is more problematic than the latter.
Maybe we should just infer a more specific type, based on the stored value at the time of migration. This would make 2) worse, but would at least fix 1)
|
Added to Cadence in onflow/cadence#3519 |
Instead of skipping the capabilities with missing borrow-type, try to infer it as either
&AnyResourceor&AnyStruct, and continue migrating such values.