Merge pull request #3154 from onflow/bastian/panic-go-runtime-errors

Improve errors in state migration

migrations/capcons/migration_test.go

@@ -70,16 +70,9 @@ type testMigration struct {
 	migration     string
 }
 
-type testMigrationError struct {
-	storageKey    interpreter.StorageKey
-	storageMapKey interpreter.StorageMapKey
-	migration     string
-	err           error
-}
-
 type testMigrationReporter struct {
 	migrations               []testMigration
-	errors                   []testMigrationError
+	errors                   []error
 	linkMigrations           []testCapConsLinkMigration
 	pathCapabilityMigrations []testCapConsPathCapabilityMigration
 	missingCapabilityIDs     []testCapConsMissingCapabilityID
@@ -106,21 +99,8 @@ func (t *testMigrationReporter) Migrated(
 	)
 }
 
-func (t *testMigrationReporter) Error(
-	storageKey interpreter.StorageKey,
-	storageMapKey interpreter.StorageMapKey,
-	migration string,
-	err error,
-) {
-	t.errors = append(
-		t.errors,
-		testMigrationError{
-			storageKey:    storageKey,
-			storageMapKey: storageMapKey,
-			migration:     migration,
-			err:           err,
-		},
-	)
+func (t *testMigrationReporter) Error(err error) {
+	t.errors = append(t.errors, err)
 }
 func (t *testMigrationReporter) MigratedLink(
 	accountAddressPath interpreter.AddressPath,
@@ -252,7 +232,7 @@ func testPathCapabilityValueMigration(
 	pathLinks []testLink,
 	accountLinks []interpreter.PathValue,
 	expectedMigrations []testMigration,
-	expectedErrors []testMigrationError,
+	expectedErrors []error,
 	expectedPathMigrations []testCapConsPathCapabilityMigration,
 	expectedMissingCapabilityIDs []testCapConsMissingCapabilityID,
 	setupFunction, checkFunction string,
@@ -583,7 +563,7 @@ func TestPathCapabilityValueMigration(t *testing.T) {
 		pathLinks                    []testLink
 		accountLinks                 []interpreter.PathValue
 		expectedMigrations           []testMigration
-		expectedErrors               []testMigrationError
+		expectedErrors               []error
 		expectedPathMigrations       []testCapConsPathCapabilityMigration
 		expectedMissingCapabilityIDs []testCapConsMissingCapabilityID
 		borrowShouldFail             bool
@@ -1240,7 +1220,7 @@ func testLinkMigration(
 	pathLinks []testLink,
 	accountLinks []interpreter.PathValue,
 	expectedMigrations []testMigration,
-	expectedErrors []testMigrationError,
+	expectedErrors []error,
 	expectedLinkMigrations []testCapConsLinkMigration,
 	expectedCyclicLinkErrors []CyclicLinkError,
 	expectedMissingTargets []interpreter.AddressPath,
@@ -1387,7 +1367,7 @@ func TestLinkMigration(t *testing.T) {
 		pathLinks                []testLink
 		accountLinks             []interpreter.PathValue
 		expectedMigrations       []testMigration
-		expectedErrors           []testMigrationError
+		expectedErrors           []error
 		expectedLinkMigrations   []testCapConsLinkMigration
 		expectedCyclicLinkErrors []CyclicLinkError
 		expectedMissingTargets   []interpreter.AddressPath

migrations/entitlements/migration_test.go

@@ -3029,10 +3029,7 @@ type testReporter struct {
 		interpreter.StorageKey
 		interpreter.StorageMapKey
 	}]struct{}
-	errors map[struct {
-		interpreter.StorageKey
-		interpreter.StorageMapKey
-	}][]error
+	errors []error
 }
 
 func newTestReporter() *testReporter {
@@ -3041,10 +3038,6 @@ func newTestReporter() *testReporter {
 			interpreter.StorageKey
 			interpreter.StorageMapKey
 		}]struct{}{},
-		errors: map[struct {
-			interpreter.StorageKey
-			interpreter.StorageMapKey
-		}][]error{},
 	}
 }
 
@@ -3062,24 +3055,8 @@ func (t *testReporter) Migrated(
 	}] = struct{}{}
 }
 
-func (t *testReporter) Error(
-	storageKey interpreter.StorageKey,
-	storageMapKey interpreter.StorageMapKey,
-	_ string,
-	err error,
-) {
-	key := struct {
-		interpreter.StorageKey
-		interpreter.StorageMapKey
-	}{
-		StorageKey:    storageKey,
-		StorageMapKey: storageMapKey,
-	}
-
-	t.errors[key] = append(
-		t.errors[key],
-		err,
-	)
+func (t *testReporter) Error(err error) {
+	t.errors = append(t.errors, err)
 }
 
 func TestRehash(t *testing.T) {

migrations/migration.go

@@ -19,6 +19,9 @@
 package migrations
 
 import (
+	"fmt"
+	"runtime/debug"
+
 	"github.com/onflow/cadence/runtime"
 	"github.com/onflow/cadence/runtime/common"
 	"github.com/onflow/cadence/runtime/errors"
@@ -141,25 +144,28 @@ func (m *StorageMigration) MigrateNestedValue(
 ) (migratedValue interpreter.Value) {
 
 	defer func() {
-		//	Here it catches the panics that may occur at the framework level,
+		// Here it catches the panics that may occur at the framework level,
 		// even before going to each individual migration. e.g: iterating the dictionary for elements.
 		//
 		// There is a similar recovery at the `StorageMigration.migrate()` method,
 		// which handles panics from each individual migrations (e.g: capcon migration, static type migration, etc.).
 
 		if r := recover(); r != nil {
-			switch r := r.(type) {
-			case error:
-				if reporter != nil {
-					reporter.Error(
-						storageKey,
-						storageMapKey,
-						"StorageMigration",
-						r,
-					)
-				}
-			default:
-				panic(r)
+			err, ok := r.(error)
+			if !ok {
+				err = fmt.Errorf("%v", r)
+			}
+
+			err = StorageMigrationError{
+				StorageKey:    storageKey,
+				StorageMapKey: storageMapKey,
+				Migration:     "StorageMigration",
+				Err:           err,
+				Stack:         debug.Stack(),
+			}
+
+			if reporter != nil {
+				reporter.Error(err)
 			}
 		}
 	}()
@@ -390,12 +396,16 @@ func (m *StorageMigration) MigrateNestedValue(
 
 		if err != nil {
 			if reporter != nil {
-				reporter.Error(
-					storageKey,
-					storageMapKey,
-					migration.Name(),
-					err,
-				)
+				if _, ok := err.(StorageMigrationError); !ok {
+					err = StorageMigrationError{
+						StorageKey:    storageKey,
+						StorageMapKey: storageMapKey,
+						Migration:     migration.Name(),
+						Err:           err,
+					}
+				}
+
+				reporter.Error(err)
 			}
 			continue
 		}
@@ -436,12 +446,34 @@ func (m *StorageMigration) MigrateNestedValue(
 
 }
 
+type StorageMigrationError struct {
+	StorageKey    interpreter.StorageKey
+	StorageMapKey interpreter.StorageMapKey
+	Migration     string
+	Err           error
+	Stack         []byte
+}
+
+func (e StorageMigrationError) Error() string {
+	return fmt.Sprintf(
+		"failed to perform migration %s for %s, %s: %s\n%s",
+		e.Migration,
+		e.StorageKey,
+		e.StorageMapKey,
+		e.Err.Error(),
+		e.Stack,
+	)
+}
+
 func (m *StorageMigration) migrate(
 	migration ValueMigration,
 	storageKey interpreter.StorageKey,
 	storageMapKey interpreter.StorageMapKey,
 	value interpreter.Value,
-) (converted interpreter.Value, err error) {
+) (
+	converted interpreter.Value,
+	err error,
+) {
 
 	// Handles panics from each individual migrations (e.g: capcon migration, static type migration, etc.).
 	// So even if one migration panics, others could still run (i.e: panics are caught inside the loop).
@@ -450,11 +482,18 @@ func (m *StorageMigration) migrate(
 	// They are caught at `StorageMigration.MigrateNestedValue()`.
 	defer func() {
 		if r := recover(); r != nil {
-			switch r := r.(type) {
-			case error:
-				err = r
-			default:
-				panic(r)
+			var ok bool
+			err, ok = r.(error)
+			if !ok {
+				err = fmt.Errorf("%v", r)
+			}
+
+			err = StorageMigrationError{
+				StorageKey:    storageKey,
+				StorageMapKey: storageMapKey,
+				Migration:     migration.Name(),
+				Err:           err,
+				Stack:         debug.Stack(),
 			}
 		}
 	}()

migrations/migration_reporter.go

@@ -26,10 +26,5 @@ type Reporter interface {
 		storageMapKey interpreter.StorageMapKey,
 		migration string,
 	)
-	Error(
-		storageKey interpreter.StorageKey,
-		storageMapKey interpreter.StorageMapKey,
-		migration string,
-		err error,
-	)
+	Error(err error)
 }