Storage Fees - Storage Updates #76
Conversation
janezpodhostnik
changed the title
This comment has been minimized.
This comment has been minimized.
… janez/storage-fees-storage-updates
|
Converting to draft, because I want to move the logic to accounts.go |
… janez/storage-fees-storage-updates
… janez/storage-fees-storage-updates
janezpodhostnik
added this to the Sprint 28 - Post Beta Mainnet - Part 12 [Dec 07-Dec 18] milestone
… janez/storage-fees-storage-updates
| // Delete records a deletion in this delta. | ||
| func (d Delta) Delete(owner, controller, key string) { | ||
| k := toString(owner, controller, key) | ||
| d.Data[k] = flow.RegisterEntry{ | ||
| Key: toRegisterID(owner, controller, key), | ||
| Value: nil, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Are we sure about removing this? I thought we kept this as part of the API for the future supporting of deletion
There was a problem hiding this comment.
The delete on the delta was not used anywhere except in the delete of the view. I figured these were two code paths that are doing the same thing, so any changes in one would also need to be made on the other. That is why I thought removing the explicit delete would simplify the code.
I didn't know about any future plan for supporting deletion. I can re-add this.
There was a problem hiding this comment.
if we are not using it all good on my side
| if isController { | ||
| a.ledger.Set(string(address.Bytes()), string(address.Bytes()), key, value) | ||
| } else { | ||
| a.ledger.Set(string(address.Bytes()), "", key, value) | ||
| } |
There was a problem hiding this comment.
is the controller still a thing we need to cover from the past @m4ksio ?
There was a problem hiding this comment.
I don't think so, we should be able to get rid of it (we can start just by setting it to "" in this PR)
| v, _ := e.accounts.GetValue( | ||
| flow.BytesToAddress(owner), | ||
| string(key), | ||
| ) |
There was a problem hiding this comment.
This assumes every register is owned by an account, are we okey with this assumption?
There was a problem hiding this comment.
Not all registers are owned by addresses, but all owners that go currently through this function are addresses. I think that the owner here was always intended to be an address. The comments even state so
// GetValue gets a value for the given key in the storage, owned by the given account.
GetValue(owner, key []byte) (value []byte, err error)
// SetValue sets a value for the given key in the storage, owned by the given account.
SetValue(owner, key, value []byte) (err error)I wanted to change the interface in cadence to accept an address instead of a byte array in a separate PR (before merging the storage feature branch into master).
@turbolent do you think there is a problem with owner having the type of Address in these two functions?
There was a problem hiding this comment.
When we started working on the interface we didn’t quite know yet if we would ever have another kind of key, so we used the lowest level type possible, a byte array.
I think it's OK to change this now, though it'll be a breaking change – given that the interface is only implemented/used by Flow code, and no users are using it yet, that should be fine though.
… janez/storage-fees-storage-updates
… janez/storage-fees-storage-updates
| return err | ||
| } | ||
|
|
||
| sizeChange := int64(len(value) - len(oldValue)) |
There was a problem hiding this comment.
This is not using the key part as part of storage used yet. I want to add that in a different PR as it needs a few more changes.
ref: onflow/flow#99
storage_usedwhen updating register values.storage_usedregister when creating a new account