FLIP-0099: Storage Fees #99
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/onflow/flow-docs/E6KSLP3GDKXmu79bJxmvUMhc6V23 |
flips/20201310-storage-fees.md
Outdated
| - absolute: get all registers for each address (touched in the current `View`) and sum their size to get `storage_used` | ||
| - differential: on the current `View`, keep track of register size changes per address and sum them with the previous `storage_used`. | ||
|
|
||
| The decision was made to go with the differential approach, because iterating through all key value pairs in storage for each account touched would be to expensive and would not scale well. |
There was a problem hiding this comment.
Nice, this choice makes sense 👍
flips/20201310-storage-fees.md
Outdated
| 2. NewTransactionSequenceNumberChecker | ||
| 3. NewTransactionFeeDeductor | ||
| 4. NewTransactionInvocator <- actual transaction | ||
| 5. *There are no further register (view) writes (or deletes) after this point. All fungible token moves already happened. All storage size changes already happened* |
flips/20201310-storage-fees.md
Outdated
| 3. NewTransactionFeeDeductor | ||
| 4. NewTransactionInvocator <- actual transaction | ||
| 5. *There are no further register (view) writes (or deletes) after this point. All fungible token moves already happened. All storage size changes already happened* | ||
| 6. **NewStorageLimiter** <- for all participating account that have touched registers, compute account size (if over limit raise error) |
There was a problem hiding this comment.
Nice! I'm glad the transaction pipeline will be put to use.
There was a problem hiding this comment.
Just learned about this, great idea to make it extensible like that! 👏
flips/20201310-storage-fees.md
Outdated
|
|
||
| The basic principle of this approach is to update accounts `storage_used` every time a register size changes, by adding the size change to current `storage_used`. | ||
|
|
||
| - During `View.Set` and `View.Delete` if storage size changes update `storage_used`. |
There was a problem hiding this comment.
Will this require an extra read before every write?
I suppose in most cases, a register is either being written for the first time or is being written after previously being read earlier in the transaction. This is getting into implementation details, but I wonder if the View could keep a clever cache of register sizes as it performs every read in order to prevent future redundant lookups.
There was a problem hiding this comment.
A cache to reduce reads is a good idea. It also reduces the amount of times the spock needs to be updated, which is one of the concerns I had.
There was a problem hiding this comment.
Reads are used to generate SPoCKs so if cache is to be used it must be deterministic
There was a problem hiding this comment.
the order and number of times a register is read is an important source of entropy for SPoCKs, if we want to put it behind a cache we need @tarakby's approval.
There was a problem hiding this comment.
If we don't have it already, I think we should add a cheap look-up to check the size of each storage register without loading its contents. This is going to be useful for LOTS of things (for example, any wallet software that helps a user manage their storage usage when their account starts to fill up!). To be clear, I'm not suggesting we need to expose this to Cadence yet (although that would be awesome), but we might as well build it internally for now, knowing that it will be useful to expose externally later.
Co-authored-by: Peter Siemens <peterjsiemens@gmail.com>
| with this the migration pseudo-code can be written as: | ||
|
|
||
| ```py | ||
| def migrate(key, value) -> (keyType, valueType)[]: |
There was a problem hiding this comment.
nice work on using python for explaining the logic. 👏
There was a problem hiding this comment.
This is great, well done @janezpodhostnik! 👏
I've added some suggestions, but nothing major, they're all minor/cosmetic
flips/20201310-storage-fees.md
Outdated
| - the payer pays for the minimum storage needed by the address | ||
| - checks if `storage_capacity` is already present on the address (if so panic) | ||
| - sets the `storage_capacity` to `minimumAccountStorage` | ||
| - using `FlowFees` deducts `flowPerAccountCreation` from the payer |
There was a problem hiding this comment.
I would maintain that only base transaction fees (the cost of inclusion and computation) should be implicitly paid by the payer. All other fees (including storage costs and account creation) should be explicitly paid by passing in a FLOW vault. Of course, the payer can pay these amounts, but we shouldn't require it.
(BTW- It seems like we currently charge the payer implicitly for creating a new account, which I consider incorrect. Personally, I feel strongly enough about this that I'd rather have account creation and storage payment be inconsistent rather than move us towards having more things paid for implicitly. Assuming, of course, we don't have time to change the way account creation is paid for.)
laynelafrance
added this to the Sprint 29 - Post Beta Mainnet - Part 12 [Jan 04-Jan 15] milestone
|
@janezpodhostnik Maybe we can merge this now, given that it is now implemented and released? |
Smart contract is here: onflow/flow-core-contracts#76
Documentation task is here: #194