Merge pull request #3522 from onflow/supun/improve-reporting

Improve storage path capability migration reporting
onflow · Aug 13, 2024 · 89cd25b · 89cd25b
2 parents 02f3a95 + 7f0f77b
commit 89cd25b
Show file tree

Hide file tree

Showing 3 changed files with 84 additions and 32 deletions.
diff --git a/migrations/capcons/capabilities.go b/migrations/capcons/capabilities.go
@@ -19,27 +19,43 @@
 package capcons
 
 import (
+	"fmt"
 	"sync"
 
 	"github.com/onflow/cadence/runtime/common"
 	"github.com/onflow/cadence/runtime/interpreter"
 )
 
 type AccountCapability struct {
-	Path       interpreter.PathValue
+	TargetPath interpreter.PathValue
 	BorrowType interpreter.StaticType
+	StoredPath Path
+}
+
+type Path struct {
+	Domain string
+	Path   string
 }
 
 type AccountCapabilities struct {
 	Capabilities []AccountCapability
 }
 
-func (c *AccountCapabilities) Record(path interpreter.PathValue, borrowType interpreter.StaticType) {
+func (c *AccountCapabilities) Record(
+	path interpreter.PathValue,
+	borrowType interpreter.StaticType,
+	storageKey interpreter.StorageKey,
+	storageMapKey interpreter.StorageMapKey,
+) {
 	c.Capabilities = append(
 		c.Capabilities,
 		AccountCapability{
-			Path:       path,
+			TargetPath: path,
 			BorrowType: borrowType,
+			StoredPath: Path{
+				Domain: storageKey.Key,
+				Path:   fmt.Sprintf("%s", storageMapKey),
+			},
 		},
 	)
 }
@@ -52,6 +68,8 @@ type AccountsCapabilities struct {
 func (m *AccountsCapabilities) Record(
 	addressPath interpreter.AddressPath,
 	borrowType interpreter.StaticType,
+	storageKey interpreter.StorageKey,
+	storageMapKey interpreter.StorageMapKey,
 ) {
 	var accountCapabilities *AccountCapabilities
 	rawAccountCapabilities, ok := m.accountCapabilities.Load(addressPath.Address)
@@ -61,7 +79,12 @@ func (m *AccountsCapabilities) Record(
 		accountCapabilities = &AccountCapabilities{}
 		m.accountCapabilities.Store(addressPath.Address, accountCapabilities)
 	}
-	accountCapabilities.Record(addressPath.Path, borrowType)
+	accountCapabilities.Record(
+		addressPath.Path,
+		borrowType,
+		storageKey,
+		storageMapKey,
+	)
 }
 
 func (m *AccountsCapabilities) ForEach(

diff --git a/migrations/capcons/migration_test.go b/migrations/capcons/migration_test.go
@@ -3178,22 +3178,34 @@ func TestStorageCapMigration(t *testing.T) {
 			{
 				address: testAddress,
 				capability: AccountCapability{
-					Path:       testPath,
+					TargetPath: testPath,
 					BorrowType: testBorrowType1,
+					StoredPath: Path{
+						Domain: "storage",
+						Path:   "cap1",
+					},
 				},
 			},
 			{
 				address: testAddress,
 				capability: AccountCapability{
-					Path:       testPath,
+					TargetPath: testPath,
 					BorrowType: testBorrowType2,
+					StoredPath: Path{
+						Domain: "storage",
+						Path:   "cap3",
+					},
 				},
 			},
 			{
 				address: testAddress,
 				capability: AccountCapability{
-					Path:       testPath,
+					TargetPath: testPath,
 					BorrowType: testBorrowType2,
+					StoredPath: Path{
+						Domain: "storage",
+						Path:   "cap2",
+					},
 				},
 			},
 		},
@@ -3442,7 +3454,11 @@ func TestUntypedStorageCapMigration(t *testing.T) {
 			{
 				address: testAddress,
 				capability: AccountCapability{
-					Path: testPath,
+					TargetPath: testPath,
+					StoredPath: Path{
+						Domain: "storage",
+						Path:   "cap",
+					},
 				},
 			},
 		},
@@ -3453,22 +3469,23 @@ func TestUntypedStorageCapMigration(t *testing.T) {
 func TestUntypedStorageCapWithMissingTargetMigration(t *testing.T) {
 	t.Parallel()
 
-	testPath := interpreter.PathValue{
+	addressA := common.MustBytesToAddress([]byte{0x1})
+	addressB := common.MustBytesToAddress([]byte{0x2})
+
+	targetPath := interpreter.PathValue{
 		Domain:     common.PathDomainStorage,
 		Identifier: testPathIdentifier,
 	}
 
-	testAddressPath := interpreter.AddressPath{
-		Address: testAddress,
-		Path:    testPath,
-	}
+	// Capability targets `addressB`.
+	// Capability itself is stored in `addressA`
 
 	capabilityValue := &interpreter.PathCapabilityValue{ //nolint:staticcheck
 		// Borrow type must be nil.
 		BorrowType: nil,
 
-		Path:    testPath,
-		Address: interpreter.AddressValue(testAddress),
+		Path:    targetPath,
+		Address: interpreter.AddressValue(addressB),
 	}
 
 	rt := NewTestInterpreterRuntime()
@@ -3478,7 +3495,7 @@ func TestUntypedStorageCapWithMissingTargetMigration(t *testing.T) {
 	runtimeInterface := &TestRuntimeInterface{
 		Storage: NewTestLedger(nil, nil),
 		OnGetSigningAccounts: func() ([]runtime.Address, error) {
-			return []runtime.Address{testAddress}, nil
+			return []runtime.Address{addressA}, nil
 		},
 		OnEmitEvent: func(event cadence.Event) error {
 			events = append(events, event)
@@ -3540,7 +3557,7 @@ func TestUntypedStorageCapWithMissingTargetMigration(t *testing.T) {
 	})
 	require.NoError(t, err)
 
-	migration, err := migrations.NewStorageMigration(inter, storage, "test", testAddress)
+	migration, err := migrations.NewStorageMigration(inter, storage, "test", addressA)
 	require.NoError(t, err)
 
 	reporter := &testMigrationReporter{}
@@ -3558,14 +3575,14 @@ func TestUntypedStorageCapWithMissingTargetMigration(t *testing.T) {
 		),
 	)
 
-	storageCapabilities := storageDomainCapabilities.Get(testAddress)
+	storageCapabilities := storageDomainCapabilities.Get(addressB)
 	require.NotNil(t, storageCapabilities)
 
 	IssueAccountCapabilities(
 		inter,
 		storage,
 		reporter,
-		testAddress,
+		addressA,
 		storageCapabilities,
 		handler,
 		typedStorageCapabilityMapping,
@@ -3601,8 +3618,11 @@ func TestUntypedStorageCapWithMissingTargetMigration(t *testing.T) {
 		t,
 		[]testCapConsMissingCapabilityID{
 			{
-				accountAddress: testAddress,
-				addressPath:    testAddressPath,
+				accountAddress: addressA,
+				addressPath: interpreter.AddressPath{
+					Address: addressB,
+					Path:    targetPath,
+				},
 			},
 		},
 		reporter.missingCapabilityIDs,
@@ -3612,8 +3632,11 @@ func TestUntypedStorageCapWithMissingTargetMigration(t *testing.T) {
 		t,
 		[]testStorageCapConsMissingBorrowType{
 			{
-				accountAddress: testAddress,
-				addressPath:    testAddressPath,
+				accountAddress: addressA,
+				addressPath: interpreter.AddressPath{
+					Address: addressA,
+					Path:    targetPath,
+				},
 			},
 		},
 		reporter.missingStorageCapConBorrowTypes,
@@ -3630,12 +3653,12 @@ func TestUntypedStorageCapWithMissingTargetMigration(t *testing.T) {
 	var actuals []actual
 
 	storageDomainCapabilities.ForEach(
-		testAddress,
+		addressB,
 		func(accountCapability AccountCapability) bool {
 			actuals = append(
 				actuals,
 				actual{
-					address:    testAddress,
+					address:    addressA,
 					capability: accountCapability,
 				},
 			)
@@ -3646,9 +3669,13 @@ func TestUntypedStorageCapWithMissingTargetMigration(t *testing.T) {
 	assert.Equal(t,
 		[]actual{
 			{
-				address: testAddress,
+				address: addressA,
 				capability: AccountCapability{
-					Path: testPath,
+					TargetPath: targetPath,
+					StoredPath: Path{
+						Domain: "storage",
+						Path:   "cap",
+					},
 				},
 			},
 		},

diff --git a/migrations/capcons/storagecapmigration.go b/migrations/capcons/storagecapmigration.go
@@ -63,8 +63,8 @@ func (*StorageCapMigration) Domains() map[string]struct{} {
 // Migrate records path capabilities with storage domain target.
 // It does not actually migrate any values.
 func (m *StorageCapMigration) Migrate(
-	_ interpreter.StorageKey,
-	_ interpreter.StorageMapKey,
+	storageKey interpreter.StorageKey,
+	storageMapKey interpreter.StorageMapKey,
 	value interpreter.Value,
 	_ *interpreter.Interpreter,
 	_ migrations.ValueMigrationPosition,
@@ -79,6 +79,8 @@ func (m *StorageCapMigration) Migrate(
 		m.StorageDomainCapabilities.Record(
 			pathCapabilityValue.AddressPath(),
 			pathCapabilityValue.BorrowType,
+			storageKey,
+			storageMapKey,
 		)
 	}
 
@@ -111,7 +113,7 @@ func IssueAccountCapabilities(
 
 		addressPath := interpreter.AddressPath{
 			Address: address,
-			Path:    capability.Path,
+			Path:    capability.TargetPath,
 		}
 
 		capabilityBorrowType := capability.BorrowType
@@ -132,7 +134,7 @@ func IssueAccountCapabilities(
 			}
 
 			// If the borrow type is missing, then borrow it as the type of the value.
-			path := capability.Path.Identifier
+			path := capability.TargetPath.Identifier
 			value := storageMap.ReadValue(nil, interpreter.StringStorageMapKey(path))
 
 			// However, if there is no value at the target,
@@ -159,7 +161,7 @@ func IssueAccountCapabilities(
 			handler,
 			address,
 			borrowType,
-			capability.Path,
+			capability.TargetPath,
 		)
 
 		if hasBorrowType {