From 7f68e0666b8538c3c9fc2f79f5d992930ea62b88 Mon Sep 17 00:00:00 2001 From: Supun Setunga Date: Mon, 19 Aug 2024 17:24:02 -0700 Subject: [PATCH 1/3] Make capabilities iteration sorted --- migrations/capcons/capabilities.go | 38 ++++++++-- migrations/capcons/capabilities_test.go | 88 +++++++++++++++++++++++ migrations/capcons/storagecapmigration.go | 12 ++-- 3 files changed, 128 insertions(+), 10 deletions(-) create mode 100644 migrations/capcons/capabilities_test.go diff --git a/migrations/capcons/capabilities.go b/migrations/capcons/capabilities.go index f33f1e9bd2..411c4f1573 100644 --- a/migrations/capcons/capabilities.go +++ b/migrations/capcons/capabilities.go @@ -20,8 +20,11 @@ package capcons import ( "fmt" + "strings" "sync" + "golang.org/x/exp/slices" + "github.com/onflow/cadence/runtime/common" "github.com/onflow/cadence/runtime/interpreter" ) @@ -60,6 +63,34 @@ func (c *AccountCapabilities) Record( ) } +func (c *AccountCapabilities) ForEach( + f func(AccountCapability) bool, +) { + slices.SortFunc( + c.Capabilities, + func(a, b AccountCapability) int { + pathA := a.TargetPath + pathB := b.TargetPath + + if pathA.Domain == pathB.Domain { + return strings.Compare(pathA.Identifier, pathB.Identifier) + } + + if pathA.Domain < pathB.Domain { + return -1 + } + + return +1 + }, + ) + + for _, accountCapability := range c.Capabilities { + if !f(accountCapability) { + return + } + } +} + type AccountsCapabilities struct { // accountCapabilities maps common.Address to *AccountCapabilities accountCapabilities sync.Map @@ -97,11 +128,8 @@ func (m *AccountsCapabilities) ForEach( } accountCapabilities := rawAccountCapabilities.(*AccountCapabilities) - for _, accountCapability := range accountCapabilities.Capabilities { - if !f(accountCapability) { - return - } - } + + accountCapabilities.ForEach(f) } func (m *AccountsCapabilities) Get(address common.Address) *AccountCapabilities { diff --git a/migrations/capcons/capabilities_test.go b/migrations/capcons/capabilities_test.go new file mode 100644 index 0000000000..d683be9a04 --- /dev/null +++ b/migrations/capcons/capabilities_test.go @@ -0,0 +1,88 @@ +/* + * Cadence - The resource-oriented smart contract programming language + * + * Copyright Flow Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package capcons + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/onflow/cadence/runtime/common" + "github.com/onflow/cadence/runtime/interpreter" +) + +func TestCapabilitiesIteration(t *testing.T) { + t.Parallel() + + caps := AccountCapabilities{} + + caps.Record( + interpreter.NewUnmeteredPathValue(common.PathDomainPublic, "b"), + nil, + interpreter.StorageKey{}, + nil, + ) + + caps.Record( + interpreter.NewUnmeteredPathValue(common.PathDomainPublic, "a"), + nil, + interpreter.StorageKey{}, + nil, + ) + + caps.Record( + interpreter.NewUnmeteredPathValue(common.PathDomainStorage, "c"), + nil, + interpreter.StorageKey{}, + nil, + ) + + caps.Record( + interpreter.NewUnmeteredPathValue(common.PathDomainStorage, "a"), + nil, + interpreter.StorageKey{}, + nil, + ) + + caps.Record( + interpreter.NewUnmeteredPathValue(common.PathDomainStorage, "b"), + nil, + interpreter.StorageKey{}, + nil, + ) + + var paths []interpreter.PathValue + + caps.ForEach(func(capability AccountCapability) bool { + paths = append(paths, capability.TargetPath) + return true + }) + + assert.Equal( + t, + []interpreter.PathValue{ + interpreter.NewUnmeteredPathValue(common.PathDomainStorage, "a"), + interpreter.NewUnmeteredPathValue(common.PathDomainStorage, "b"), + interpreter.NewUnmeteredPathValue(common.PathDomainStorage, "c"), + interpreter.NewUnmeteredPathValue(common.PathDomainPublic, "a"), + interpreter.NewUnmeteredPathValue(common.PathDomainPublic, "b"), + }, + paths, + ) +} diff --git a/migrations/capcons/storagecapmigration.go b/migrations/capcons/storagecapmigration.go index ba95c1dd16..2118ab7b50 100644 --- a/migrations/capcons/storagecapmigration.go +++ b/migrations/capcons/storagecapmigration.go @@ -109,7 +109,7 @@ func IssueAccountCapabilities( false, ) - for _, capability := range capabilities.Capabilities { + capabilities.ForEach(func(capability AccountCapability) bool { addressPath := interpreter.AddressPath{ Address: address, @@ -123,7 +123,7 @@ func IssueAccountCapabilities( if hasBorrowType { if _, ok := typedCapabilityMapping.Get(addressPath, capabilityBorrowType.ID()); ok { - continue + return true } borrowType = capabilityBorrowType.(*interpreter.ReferenceStaticType) @@ -141,7 +141,7 @@ func IssueAccountCapabilities( reporter.MissingBorrowType(addressPath, targetPath) if _, _, ok := untypedCapabilityMapping.Get(addressPath); ok { - continue + return true } // If the borrow type is missing, then borrow it as the type of the value. @@ -151,7 +151,7 @@ func IssueAccountCapabilities( // However, if there is no value at the target, //it is not possible to migrate this cap. if value == nil { - continue + return true } valueType := value.StaticType(inter) @@ -198,5 +198,7 @@ func IssueAccountCapabilities( borrowType, capabilityID, ) - } + + return true + }) } From f712fdea18aa174f9073a26156c7d101f891faba Mon Sep 17 00:00:00 2001 From: Supun Setunga Date: Mon, 19 Aug 2024 18:31:10 -0700 Subject: [PATCH 2/3] Refactor code --- migrations/capcons/capabilities.go | 54 +++++++++++++---------- migrations/capcons/capabilities_test.go | 2 +- migrations/capcons/storagecapmigration.go | 2 +- 3 files changed, 32 insertions(+), 26 deletions(-) diff --git a/migrations/capcons/capabilities.go b/migrations/capcons/capabilities.go index 411c4f1573..8b9314a46e 100644 --- a/migrations/capcons/capabilities.go +++ b/migrations/capcons/capabilities.go @@ -19,6 +19,7 @@ package capcons import ( + "cmp" "fmt" "strings" "sync" @@ -41,7 +42,8 @@ type Path struct { } type AccountCapabilities struct { - Capabilities []AccountCapability + capabilities []AccountCapability + sortOnce sync.Once } func (c *AccountCapabilities) Record( @@ -50,8 +52,8 @@ func (c *AccountCapabilities) Record( storageKey interpreter.StorageKey, storageMapKey interpreter.StorageMapKey, ) { - c.Capabilities = append( - c.Capabilities, + c.capabilities = append( + c.capabilities, AccountCapability{ TargetPath: path, BorrowType: borrowType, @@ -63,34 +65,38 @@ func (c *AccountCapabilities) Record( ) } -func (c *AccountCapabilities) ForEach( +// ForEachSorted will first sort the capabilities list, +// and iterates through the sorted list. +func (c *AccountCapabilities) ForEachSorted( f func(AccountCapability) bool, ) { - slices.SortFunc( - c.Capabilities, - func(a, b AccountCapability) int { - pathA := a.TargetPath - pathB := b.TargetPath - - if pathA.Domain == pathB.Domain { - return strings.Compare(pathA.Identifier, pathB.Identifier) - } - - if pathA.Domain < pathB.Domain { - return -1 - } - - return +1 - }, - ) - - for _, accountCapability := range c.Capabilities { + c.sort() + for _, accountCapability := range c.capabilities { if !f(accountCapability) { return } } } +func (c *AccountCapabilities) sort() { + c.sortOnce.Do( + func() { + slices.SortFunc( + c.capabilities, + func(a, b AccountCapability) int { + pathA := a.TargetPath + pathB := b.TargetPath + + return cmp.Or( + cmp.Compare(pathA.Domain, pathB.Domain), + strings.Compare(pathA.Identifier, pathB.Identifier), + ) + }, + ) + }, + ) +} + type AccountsCapabilities struct { // accountCapabilities maps common.Address to *AccountCapabilities accountCapabilities sync.Map @@ -129,7 +135,7 @@ func (m *AccountsCapabilities) ForEach( accountCapabilities := rawAccountCapabilities.(*AccountCapabilities) - accountCapabilities.ForEach(f) + accountCapabilities.ForEachSorted(f) } func (m *AccountsCapabilities) Get(address common.Address) *AccountCapabilities { diff --git a/migrations/capcons/capabilities_test.go b/migrations/capcons/capabilities_test.go index d683be9a04..d12599f39e 100644 --- a/migrations/capcons/capabilities_test.go +++ b/migrations/capcons/capabilities_test.go @@ -69,7 +69,7 @@ func TestCapabilitiesIteration(t *testing.T) { var paths []interpreter.PathValue - caps.ForEach(func(capability AccountCapability) bool { + caps.ForEachSorted(func(capability AccountCapability) bool { paths = append(paths, capability.TargetPath) return true }) diff --git a/migrations/capcons/storagecapmigration.go b/migrations/capcons/storagecapmigration.go index 2118ab7b50..4131f7c1ec 100644 --- a/migrations/capcons/storagecapmigration.go +++ b/migrations/capcons/storagecapmigration.go @@ -109,7 +109,7 @@ func IssueAccountCapabilities( false, ) - capabilities.ForEach(func(capability AccountCapability) bool { + capabilities.ForEachSorted(func(capability AccountCapability) bool { addressPath := interpreter.AddressPath{ Address: address, From 2d6e436046266ff1de4d31d75fd50457eb9d71f0 Mon Sep 17 00:00:00 2001 From: Supun Setunga Date: Tue, 20 Aug 2024 09:05:57 -0700 Subject: [PATCH 3/3] Switch from sync.Once to a boolean flag --- migrations/capcons/capabilities.go | 33 ++++++++++++++---------- migrations/capcons/capabilities_test.go | 34 +++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 14 deletions(-) diff --git a/migrations/capcons/capabilities.go b/migrations/capcons/capabilities.go index 8b9314a46e..8b06c93794 100644 --- a/migrations/capcons/capabilities.go +++ b/migrations/capcons/capabilities.go @@ -43,7 +43,7 @@ type Path struct { type AccountCapabilities struct { capabilities []AccountCapability - sortOnce sync.Once + sorted bool } func (c *AccountCapabilities) Record( @@ -63,6 +63,9 @@ func (c *AccountCapabilities) Record( }, }, ) + + // Reset the sorted flag, if new entries are added. + c.sorted = false } // ForEachSorted will first sort the capabilities list, @@ -79,22 +82,24 @@ func (c *AccountCapabilities) ForEachSorted( } func (c *AccountCapabilities) sort() { - c.sortOnce.Do( - func() { - slices.SortFunc( - c.capabilities, - func(a, b AccountCapability) int { - pathA := a.TargetPath - pathB := b.TargetPath - - return cmp.Or( - cmp.Compare(pathA.Domain, pathB.Domain), - strings.Compare(pathA.Identifier, pathB.Identifier), - ) - }, + if c.sorted { + return + } + + slices.SortFunc( + c.capabilities, + func(a, b AccountCapability) int { + pathA := a.TargetPath + pathB := b.TargetPath + + return cmp.Or( + cmp.Compare(pathA.Domain, pathB.Domain), + strings.Compare(pathA.Identifier, pathB.Identifier), ) }, ) + + c.sorted = true } type AccountsCapabilities struct { diff --git a/migrations/capcons/capabilities_test.go b/migrations/capcons/capabilities_test.go index d12599f39e..b4b4869668 100644 --- a/migrations/capcons/capabilities_test.go +++ b/migrations/capcons/capabilities_test.go @@ -22,6 +22,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/onflow/cadence/runtime/common" "github.com/onflow/cadence/runtime/interpreter" @@ -67,17 +68,50 @@ func TestCapabilitiesIteration(t *testing.T) { nil, ) + require.False(t, caps.sorted) + var paths []interpreter.PathValue + caps.ForEachSorted(func(capability AccountCapability) bool { + paths = append(paths, capability.TargetPath) + return true + }) + + require.True(t, caps.sorted) + + assert.Equal( + t, + []interpreter.PathValue{ + interpreter.NewUnmeteredPathValue(common.PathDomainStorage, "a"), + interpreter.NewUnmeteredPathValue(common.PathDomainStorage, "b"), + interpreter.NewUnmeteredPathValue(common.PathDomainStorage, "c"), + interpreter.NewUnmeteredPathValue(common.PathDomainPublic, "a"), + interpreter.NewUnmeteredPathValue(common.PathDomainPublic, "b"), + }, + paths, + ) + caps.Record( + interpreter.NewUnmeteredPathValue(common.PathDomainStorage, "aa"), + nil, + interpreter.StorageKey{}, + nil, + ) + + require.False(t, caps.sorted) + + paths = make([]interpreter.PathValue, 0) caps.ForEachSorted(func(capability AccountCapability) bool { paths = append(paths, capability.TargetPath) return true }) + require.True(t, caps.sorted) + assert.Equal( t, []interpreter.PathValue{ interpreter.NewUnmeteredPathValue(common.PathDomainStorage, "a"), + interpreter.NewUnmeteredPathValue(common.PathDomainStorage, "aa"), interpreter.NewUnmeteredPathValue(common.PathDomainStorage, "b"), interpreter.NewUnmeteredPathValue(common.PathDomainStorage, "c"), interpreter.NewUnmeteredPathValue(common.PathDomainPublic, "a"),