policy: Shorten policy contract keys #71

aarifullin · 2024-01-18T08:02:10Z

aarifullin commented

2024-01-18 08:02:10 +00:00

Map long entity names to 8-bytes long numbers.
Add desctiption to docs.

Close #71

* Map long entity names to 8-bytes long numbers. * Add desctiption to docs. Close #71

aarifullin requested review from storage-core-committers 2024-01-18 08:02:27 +00:00

aarifullin requested review from storage-core-developers 2024-01-18 08:02:36 +00:00

aarifullin requested review from storage-services-committers 2024-01-18 08:02:45 +00:00

aarifullin requested review from storage-services-developers 2024-01-18 08:02:46 +00:00

dkirillov reviewed 2024-01-18 08:49:15 +00:00

policy/policy_contract.go Outdated

					
				@ -89,0 +101,4 @@

				// by AddChain invocation.

				func mapToNumeric(ctx storage.Context, name []byte) []byte {

					if len(name) <= maxAllowedEntityNameLength {

						return name

dkirillov commented

2024-01-18 08:49:15 +00:00

I'm not sure that we can use plain namespace if its length is less than 8 bytes because it seems we can conflict with mapped one.
Let's consider namespace with name a, it's []byte representation is []byte{97}, but we will get the same representation for 97 container that length is greater 8 as I can undersand

I'm not sure that we can use plain `namespace` if its length is less than 8 bytes because it seems we can conflict with mapped one. Let's consider namespace with name `a`, it's []byte representation is `[]byte{97}`, but we will get the same representation for 97 container that length is greater 8 as I can undersand

aarifullin commented

2024-01-18 09:10:10 +00:00

You're absolutely right. I haven't considered this collision. Alright, I'll remove this out then

aarifullin commented

2024-01-18 09:27:14 +00:00

Fixed. Since all names are mapped to numbers

Fixed. Since **all** names are mapped to numbers

dkirillov marked this conversation as resolved

dkirillov reviewed 2024-01-18 08:51:58 +00:00

policy/policy_contract.go Outdated

					
				@ -45,2 +50,4 @@

						storage.Put(ctx, []byte{ownerKeyPrefix}, args.Admin)

					}

					storage.Put(ctx, counterKey, uint64(0))

dkirillov commented

2024-01-18 08:51:58 +00:00

Isn't int in contracts is uint64 anyway and we can skip conversion?

Isn't `int` in contracts is `uint64` anyway and we can skip conversion?

aarifullin commented

2024-01-18 09:42:33 +00:00

I think you're right. I have replaced uint64 by int

I think you're right. I have replaced `uint64` by `int`

dkirillov marked this conversation as resolved

dkirillov reviewed 2024-01-18 08:53:06 +00:00

policy/policy_contract.go

					
				@ -82,3 +89,3 @@

				func storageKey(prefix Kind, entityName string, name []byte) []byte {

				func storageKey(prefix Kind, entityName, name []byte) []byte {

					ln := len(entityName)

					key := append([]byte{byte(prefix)}, byte(ln&0xFF), byte(ln>>8))

dkirillov commented

2024-01-18 08:53:06 +00:00

Shouldn't we simplify this to key := append([]byte{byte(prefix)}, entityName...)

Shouldn't we simplify this to `key := append([]byte{byte(prefix)}, entityName...)`

aarifullin commented

2024-01-18 09:21:19 +00:00

Appending entityName's length wasn't idea of mine (it has been introduced by @fyrchik), but I think it is fine because it helps to determine where a chain ID starts in a storage key.

Appending `entityName`'s length wasn't idea of mine (it has been introduced by @fyrchik), but I think it is fine because it helps to determine where a chain ID starts in a storage key.

dkirillov commented

2024-01-18 11:24:39 +00:00

entityName now always 8 bytes long. So we can save two bytes here if I understand it correctly

`entityName` now always 8 bytes long. So we can save two bytes here if I understand it correctly

aarifullin commented

2024-01-18 11:46:14 +00:00

Alright. I've removed this append

dkirillov marked this conversation as resolved

aarifullin force-pushed feature/61-shorten_policy_keys from 598089d3da to 9f49a70d65

2024-01-18 09:26:49 +00:00

Compare

aarifullin force-pushed feature/61-shorten_policy_keys from 9f49a70d65 to e0c9e5848f

2024-01-18 09:42:07 +00:00

Compare

aarifullin force-pushed feature/61-shorten_policy_keys from e0c9e5848f to ce7d6fed44

2024-01-18 10:45:38 +00:00

Compare

dkirillov reviewed 2024-01-18 11:21:46 +00:00

policy/policy_contract.go Outdated

					
				@ -89,0 +104,4 @@

						return nil

					}

					mapped := numericID.(int)

					return common.ToFixedWidth64(int(mapped))

dkirillov commented

2024-01-18 11:21:46 +00:00

Can be just return common.ToFixedWidth64(mapped)

Can be just `return common.ToFixedWidth64(mapped)`

👍 1

aarifullin commented

2024-01-18 11:46:00 +00:00

Fixed

dkirillov marked this conversation as resolved

aarifullin force-pushed feature/61-shorten_policy_keys from ce7d6fed44 to facd7e40fb

2024-01-18 11:45:27 +00:00

Compare

dkirillov approved these changes 2024-01-18 13:11:53 +00:00

fyrchik reviewed 2024-01-18 14:30:32 +00:00

policy/policy_contract.go Outdated

					
				@ -83,3 +88,1 @@

					ln := len(entityName)

					key := append([]byte{byte(prefix)}, byte(ln&0xFF), byte(ln>>8))

					key = append(key, entityName...)

				// storageKey recieves fixed-length entityName. See: mapToNumeric, mapToNumericCreateIfNotExists.

fyrchik commented

2024-01-18 14:28:31 +00:00

Why not calculate this inside storageKey()
The invariant is hard to enforce.
Otherwise, let's rename entityName parameter, it is counter now?

Why not calculate this inside `storageKey()` The invariant is hard to enforce. Otherwise, let's rename `entityName` parameter, it is `counter` now?

fyrchik commented

2024-01-18 14:30:23 +00:00

mapToNumeric could return int, this would solve all mentioned problems.

`mapToNumeric` could return `int`, this would solve all mentioned problems.

👍 2

aarifullin commented

2024-01-19 09:14:04 +00:00

You've asked

Why not calculate this inside storageKey()

Because:

This method is also invoked by GetChain that yields read-only context. If we consider your suggestion, then it leads to a situation when we need to make storageKey to determine whether it can create mapping or not, i.e. write to the storage or not (storage.Put) -> we need to pass two more params: ctx and isReadOnlyCtx: actually, I tried to avoid this

Fusing these methods would look like that:

func storageKeyAlt(ctx storage.Context, isReadOnlyCtx bool, prefix Kind, entityName string, name []byte) []byte {
	mKey := append([]byte{mappingKeyPrefix}, entityName...)
	var mapped int
	numericID := storage.Get(ctx, mKey)
	if numericID == nil {
		if isReadOnlyCtx {
			return nil
		}
		counter := storage.Get(ctx, counterKey).(int)
		counter++
		storage.Put(ctx, counterKey, counter)
		storage.Put(ctx, mKey, counter)
		mapped = counter
	} else {
		mapped = numericID.(int)
	}

	key := append([]byte{byte(prefix)}, common.ToFixedWidth64(mapped)...)
	return append(key, name...)
}

Are we okay with that?

You've asked > Why not calculate this inside storageKey() Because: This method is also invoked by `GetChain` that yields read-only context. If we consider your suggestion, then it leads to a situation when we need to make `storageKey` to determine whether it can create mapping or not, i.e. write to the storage or not (`storage.Put`) -> we need to pass two more params: `ctx` and `isReadOnlyCtx`: actually, I tried to avoid this Fusing these methods would look like that: ```go func storageKeyAlt(ctx storage.Context, isReadOnlyCtx bool, prefix Kind, entityName string, name []byte) []byte { mKey := append([]byte{mappingKeyPrefix}, entityName...) var mapped int numericID := storage.Get(ctx, mKey) if numericID == nil { if isReadOnlyCtx { return nil } counter := storage.Get(ctx, counterKey).(int) counter++ storage.Put(ctx, counterKey, counter) storage.Put(ctx, mKey, counter) mapped = counter } else { mapped = numericID.(int) } key := append([]byte{byte(prefix)}, common.ToFixedWidth64(mapped)...) return append(key, name...) } ``` Are we okay with that?

fyrchik commented

2024-01-19 12:06:19 +00:00

Doesn't look nice either, but we could just move common.ToFixedWidth64invocation: so that storageKey accepts int returned from mapToNumeric.

Doesn't look nice either, but we could just move `common.ToFixedWidth64`invocation: so that `storageKey` accepts `int` returned from `mapToNumeric`.

aarifullin commented

2024-01-19 13:29:16 +00:00

Alright. Since mapping methods return int and storageKey recieves int

Alright. Since mapping methods return `int` and `storageKey` recieves `int`