Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace domain string with enum for AccountStorageMap key #3677

Merged
merged 3 commits into from
Nov 13, 2024
Merged

Replace domain string with enum for AccountStorageMap key #3677

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bc2b313
Replace domain string with enum for AccountStorageMap
fxamacker Nov 12, 2024
88c31e6
Rename function convertKeyToDomain
fxamacker Nov 12, 2024
f68c5eb
Add comments and warnings about StorageDomain
fxamacker Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions common/storagedomain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,19 @@ import (
"github.com/onflow/cadence/errors"
)

// StorageDomain is used to store domain values on chain.
//
// !!! *WARNING* !!!
//
// Only add new StorageDomain by:
// - appending to the end.
//
// Only remove StorageDomain by:
// - replacing existing StorageDomain with a placeholder `_`.
//
// DO *NOT* REPLACE EXISTING STORAGEDOMAIN!
// DO *NOT* REMOVE EXISTING STORAGEDOMAIN!
// DO *NOT* INSERT NEW STORAGEDOMAIN IN BETWEEN!
type StorageDomain uint8

const (
Expand Down Expand Up @@ -54,6 +67,8 @@ const (
// StorageDomainAccountCapability is the storage domain which
// records active account capability controller IDs
StorageDomainAccountCapability

// Append new StorageDomain here (if needed).
)

var AllStorageDomains = []StorageDomain{
Expand Down
26 changes: 13 additions & 13 deletions interpreter/account_storagemap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func NewAccountStorageMapWithRootID(

// DomainExists returns true if the given domain exists in the account storage map.
func (s *AccountStorageMap) DomainExists(domain common.StorageDomain) bool {
key := StringStorageMapKey(domain.Identifier())
key := Uint64StorageMapKey(domain)
fxamacker marked this conversation as resolved.
Show resolved Hide resolved

exists, err := s.orderedMap.Has(
key.AtreeValueCompare,
Expand All @@ -99,7 +99,7 @@ func (s *AccountStorageMap) GetDomain(
domain common.StorageDomain,
createIfNotExists bool,
) *DomainStorageMap {
key := StringStorageMapKey(domain.Identifier())
key := Uint64StorageMapKey(domain)

storedValue, err := s.orderedMap.Get(
key.AtreeValueCompare,
Expand Down Expand Up @@ -135,7 +135,7 @@ func (s *AccountStorageMap) NewDomain(

domainStorageMap := NewDomainStorageMap(gauge, s.orderedMap.Storage, s.orderedMap.Address())

key := StringStorageMapKey(domain.Identifier())
key := Uint64StorageMapKey(domain)

existingStorable, err := s.orderedMap.Set(
key.AtreeValueCompare,
Expand Down Expand Up @@ -181,7 +181,7 @@ func (s *AccountStorageMap) setDomain(
) (existed bool) {
interpreter.recordStorageMutation()

key := StringStorageMapKey(domain.Identifier())
key := Uint64StorageMapKey(domain)

existingValueStorable, err := s.orderedMap.Set(
key.AtreeValueCompare,
Expand Down Expand Up @@ -218,7 +218,7 @@ func (s *AccountStorageMap) setDomain(
func (s *AccountStorageMap) removeDomain(interpreter *Interpreter, domain common.StorageDomain) (existed bool) {
interpreter.recordStorageMutation()

key := StringStorageMapKey(domain.Identifier())
key := Uint64StorageMapKey(domain)

existingKeyStorable, existingValueStorable, err := s.orderedMap.Remove(
key.AtreeValueCompare,
Expand All @@ -236,7 +236,7 @@ func (s *AccountStorageMap) removeDomain(interpreter *Interpreter, domain common

// Key

// NOTE: Key is just an atree.Value (StringAtreeValue), not an interpreter.Value,
// NOTE: Key is just an atree.Value (Uint64AtreeValue), not an interpreter.Value,
// so do not need (can) convert and not need to deep remove
interpreter.RemoveReferencedSlab(existingKeyStorable)

Expand Down Expand Up @@ -284,7 +284,7 @@ func (s *AccountStorageMap) Domains() map[common.StorageDomain]struct{} {
break
}

domain := convertKeyToDomain(k)
domain := convertAccountStorageMapKeyToStorageDomain(k)
domains[domain] = struct{}{}
}

Expand Down Expand Up @@ -326,21 +326,21 @@ func (i *AccountStorageMapIterator) Next() (common.StorageDomain, *DomainStorage
return common.StorageDomainUnknown, nil
}

key := convertKeyToDomain(k)
key := convertAccountStorageMapKeyToStorageDomain(k)

value := NewDomainStorageMapWithAtreeValue(v)

return key, value
}

func convertKeyToDomain(v atree.Value) common.StorageDomain {
key, ok := v.(StringAtreeValue)
func convertAccountStorageMapKeyToStorageDomain(v atree.Value) common.StorageDomain {
key, ok := v.(Uint64AtreeValue)
if !ok {
panic(errors.NewUnexpectedError("domain key type %T isn't expected", key))
}
domain, found := common.StorageDomainFromIdentifier(string(key))
if !found {
panic(errors.NewUnexpectedError("domain key %s isn't expected", key))
domain, err := common.StorageDomainFromUint64(uint64(key))
if err != nil {
panic(errors.NewUnexpectedError("domain key %d isn't expected: %w", key, err))
}
return domain
}
104 changes: 74 additions & 30 deletions interpreter/account_storagemap_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package interpreter_test

import (
"math/rand"
goruntime "runtime"
"slices"
"strconv"
"strings"
Expand Down Expand Up @@ -188,6 +189,8 @@ func TestAccountStorageMapCreateDomain(t *testing.T) {
require.NotNil(t, accountStorageMap)
require.Equal(t, uint64(0), accountStorageMap.Count())

accountStorageMapRootSlabID := accountStorageMap.SlabID()

for _, domain := range common.AllStorageDomains {
const createIfNotExists = true
domainStoragemap := accountStorageMap.GetDomain(nil, inter, domain, createIfNotExists)
Expand All @@ -199,7 +202,12 @@ func TestAccountStorageMapCreateDomain(t *testing.T) {

checkAccountStorageMapData(t, inter, accountStorageMap, accountValues)

CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMap.SlabID()})
CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMapRootSlabID})

err := storage.PersistentSlabStorage.FastCommit(goruntime.NumCPU())
require.NoError(t, err)

checkAccountStorageMapDataWithRawData(t, ledger.StoredValues, ledger.StorageIndices, accountStorageMapRootSlabID, accountValues)
})

t.Run("non-empty", func(t *testing.T) {
Expand All @@ -225,6 +233,8 @@ func TestAccountStorageMapCreateDomain(t *testing.T) {
const count = 10
accountStorageMap, accountValues := createAccountStorageMap(storage, inter, address, existingDomains, count, random)

accountStorageMapRootSlabID := accountStorageMap.SlabID()

for _, domain := range common.AllStorageDomains {
const createIfNotExists = true
domainStoragemap := accountStorageMap.GetDomain(nil, inter, domain, createIfNotExists)
Expand All @@ -238,7 +248,12 @@ func TestAccountStorageMapCreateDomain(t *testing.T) {

checkAccountStorageMapData(t, inter, accountStorageMap, accountValues)

CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMap.SlabID()})
CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMapRootSlabID})

err := storage.PersistentSlabStorage.FastCommit(goruntime.NumCPU())
require.NoError(t, err)

checkAccountStorageMapDataWithRawData(t, ledger.StoredValues, ledger.StorageIndices, accountStorageMapRootSlabID, accountValues)
})
}

Expand Down Expand Up @@ -271,6 +286,8 @@ func TestAccountStorageMapSetAndUpdateDomain(t *testing.T) {
require.NotNil(t, accountStorageMap)
require.Equal(t, uint64(0), accountStorageMap.Count())

accountStorageMapRootSlabID := accountStorageMap.SlabID()

const count = 10
for _, domain := range common.AllStorageDomains {

Expand All @@ -285,7 +302,12 @@ func TestAccountStorageMapSetAndUpdateDomain(t *testing.T) {

checkAccountStorageMapData(t, inter, accountStorageMap, accountValues)

CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMap.SlabID()})
CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMapRootSlabID})

err := storage.PersistentSlabStorage.FastCommit(goruntime.NumCPU())
require.NoError(t, err)

checkAccountStorageMapDataWithRawData(t, ledger.StoredValues, ledger.StorageIndices, accountStorageMapRootSlabID, accountValues)
})

t.Run("non-empty", func(t *testing.T) {
Expand All @@ -311,6 +333,8 @@ func TestAccountStorageMapSetAndUpdateDomain(t *testing.T) {
const count = 10
accountStorageMap, accountValues := createAccountStorageMap(storage, inter, address, existingDomains, count, random)

accountStorageMapRootSlabID := accountStorageMap.SlabID()

for _, domain := range common.AllStorageDomains {

domainStorageMap := interpreter.NewDomainStorageMap(nil, storage, atree.Address(address))
Expand All @@ -324,7 +348,12 @@ func TestAccountStorageMapSetAndUpdateDomain(t *testing.T) {

checkAccountStorageMapData(t, inter, accountStorageMap, accountValues)

CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMap.SlabID()})
CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMapRootSlabID})

err := storage.PersistentSlabStorage.FastCommit(goruntime.NumCPU())
require.NoError(t, err)

checkAccountStorageMapDataWithRawData(t, ledger.StoredValues, ledger.StorageIndices, accountStorageMapRootSlabID, accountValues)
})
}

Expand Down Expand Up @@ -355,14 +384,21 @@ func TestAccountStorageMapRemoveDomain(t *testing.T) {
require.NotNil(t, accountStorageMap)
require.Equal(t, uint64(0), accountStorageMap.Count())

accountStorageMapRootSlabID := accountStorageMap.SlabID()

for _, domain := range common.AllStorageDomains {
existed := accountStorageMap.WriteDomain(inter, domain, nil)
require.False(t, existed)
}

checkAccountStorageMapData(t, inter, accountStorageMap, accountValues)

CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMap.SlabID()})
CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMapRootSlabID})

err := storage.PersistentSlabStorage.FastCommit(goruntime.NumCPU())
require.NoError(t, err)

checkAccountStorageMapDataWithRawData(t, ledger.StoredValues, ledger.StorageIndices, accountStorageMapRootSlabID, accountValues)
})

t.Run("non-empty", func(t *testing.T) {
Expand All @@ -388,6 +424,8 @@ func TestAccountStorageMapRemoveDomain(t *testing.T) {
const count = 10
accountStorageMap, accountValues := createAccountStorageMap(storage, inter, address, existingDomains, count, random)

accountStorageMapRootSlabID := accountStorageMap.SlabID()

for _, domain := range common.AllStorageDomains {

existed := accountStorageMap.WriteDomain(inter, domain, nil)
Expand All @@ -398,7 +436,12 @@ func TestAccountStorageMapRemoveDomain(t *testing.T) {

checkAccountStorageMapData(t, inter, accountStorageMap, accountValues)

CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMap.SlabID()})
CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMapRootSlabID})

err := storage.PersistentSlabStorage.FastCommit(goruntime.NumCPU())
require.NoError(t, err)

checkAccountStorageMapDataWithRawData(t, ledger.StoredValues, ledger.StorageIndices, accountStorageMapRootSlabID, accountValues)
})
}

Expand Down Expand Up @@ -575,18 +618,7 @@ func TestAccountStorageMapLoadFromRootSlabID(t *testing.T) {

accountStorageMapRootSlabID, accountValues, storedValues, storageIndices := init()

ledger := NewTestLedgerWithData(nil, nil, storedValues, storageIndices)
storage := runtime.NewStorage(ledger, nil)

accountStorageMap := interpreter.NewAccountStorageMapWithRootID(storage, accountStorageMapRootSlabID)
require.Equal(t, uint64(0), accountStorageMap.Count())
require.Equal(t, accountStorageMapRootSlabID, accountStorageMap.SlabID())

inter := NewTestInterpreterWithStorage(t, storage)

checkAccountStorageMapData(t, inter, accountStorageMap, accountValues)

CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMap.SlabID()})
checkAccountStorageMapDataWithRawData(t, storedValues, storageIndices, accountStorageMapRootSlabID, accountValues)
})

t.Run("non-empty", func(t *testing.T) {
Expand Down Expand Up @@ -619,18 +651,7 @@ func TestAccountStorageMapLoadFromRootSlabID(t *testing.T) {

accountStorageMapRootSlabID, accountValues, storedValues, storageIndices := init()

ledger := NewTestLedgerWithData(nil, nil, storedValues, storageIndices)
storage := runtime.NewStorage(ledger, nil)

accountStorageMap := interpreter.NewAccountStorageMapWithRootID(storage, accountStorageMapRootSlabID)
require.Equal(t, uint64(len(existingDomains)), accountStorageMap.Count())
require.Equal(t, accountStorageMapRootSlabID, accountStorageMap.SlabID())

inter := NewTestInterpreterWithStorage(t, storage)

checkAccountStorageMapData(t, inter, accountStorageMap, accountValues)

CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMap.SlabID()})
checkAccountStorageMapDataWithRawData(t, storedValues, storageIndices, accountStorageMapRootSlabID, accountValues)
})
}

Expand Down Expand Up @@ -697,6 +718,29 @@ func writeRandomValuesToDomainStorageMap(
return domainValues
}

// checkAccountStorageMapDataWithRawData checks loaded account storage map against expected account values.
func checkAccountStorageMapDataWithRawData(
tb testing.TB,
storedValues map[string][]byte,
storageIndices map[string]uint64,
rootSlabID atree.SlabID,
expectedAccountValues accountStorageMapValues,
) {
// Create new storage from raw data
ledger := NewTestLedgerWithData(nil, nil, storedValues, storageIndices)
storage := runtime.NewStorage(ledger, nil)

inter := NewTestInterpreterWithStorage(tb, storage)

loadedAccountStorageMap := interpreter.NewAccountStorageMapWithRootID(storage, rootSlabID)
require.Equal(tb, uint64(len(expectedAccountValues)), loadedAccountStorageMap.Count())
require.Equal(tb, rootSlabID, loadedAccountStorageMap.SlabID())

checkAccountStorageMapData(tb, inter, loadedAccountStorageMap, expectedAccountValues)

CheckAtreeStorageHealth(tb, storage, []atree.SlabID{rootSlabID})
}

// checkAccountStorageMapData iterates account storage map and compares values with given expectedAccountValues.
func checkAccountStorageMapData(
tb testing.TB,
Expand Down
6 changes: 3 additions & 3 deletions runtime/runtime_memory_metering_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1073,7 +1073,7 @@ func TestRuntimeMeterEncoding(t *testing.T) {
)

require.NoError(t, err)
assert.Equal(t, 114, int(meter.getMemory(common.MemoryKindBytes)))
assert.Equal(t, 107, int(meter.getMemory(common.MemoryKindBytes)))
})

t.Run("string in loop", func(t *testing.T) {
Expand Down Expand Up @@ -1122,7 +1122,7 @@ func TestRuntimeMeterEncoding(t *testing.T) {
)

require.NoError(t, err)
assert.Equal(t, 61501, int(meter.getMemory(common.MemoryKindBytes)))
assert.Equal(t, 61494, int(meter.getMemory(common.MemoryKindBytes)))
})

t.Run("composite", func(t *testing.T) {
Expand Down Expand Up @@ -1173,6 +1173,6 @@ func TestRuntimeMeterEncoding(t *testing.T) {
)

require.NoError(t, err)
assert.Equal(t, 58369, int(meter.getMemory(common.MemoryKindBytes)))
assert.Equal(t, 58362, int(meter.getMemory(common.MemoryKindBytes)))
})
}
2 changes: 1 addition & 1 deletion runtime/runtime_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7579,7 +7579,7 @@ func TestRuntimeComputationMetring(t *testing.T) {
`,
ok: true,
hits: 3,
intensity: 115,
intensity: 108,
},
}

Expand Down
Loading