feat(store): add foundation for v2 data modelling #352
Conversation
|
very much WIP - don't look |
|
a tiny improvement suggestion: |
…ecord, field and delete
…of the bytes blob
| } | ||
|
|
||
| // Allocate a buffer for the full data (static and dynamic) | ||
| Buffer buffer = Buffer_.allocate(uint128(outputLength)); |
There was a problem hiding this comment.
Coming back to the Buffer comment - here it could be something like:
bytes memory result = new bytes(outputLength);
Buffer buffer = Buffer_.fromBytes(result);And you use Buffer to append, but return the original bytes.
I guess I'm suggesting a Slice at this point, not Buffer
| /** | ||
| * Get the length of the data for the given schema type | ||
| * (Because Solidity doesn't support constant arrays, we need to use a function) | ||
| * TODO: add more types and make it more efficient (avoid linear search) |
There was a problem hiding this comment.
avoiding both linear and binary searches is probably a good idea, something like https://github.com/dk1a/mud/blob/c4cae23ac0284d6d475b683ad145daaf748e910e/packages/solecs/v2/SchemaType.sol#L225
packages/store/src/Utils.sol
Outdated
| * @dev Left-aligned bit mask (e.g. for partial mload/mstore). | ||
| * For length >= 32 returns type(uint256).max | ||
| * | ||
| * length 0: 0x000000...000000 | ||
| * length 1: 0xff0000...000000 | ||
| * length 2: 0xffff00...000000 | ||
| * ... | ||
| * length 30: 0xffffff...ff0000 | ||
| * length 31: 0xffffff...ffff00 | ||
| * length 32+: 0xffffff...ffffff |
There was a problem hiding this comment.
Since you changed it from bytes to bits the comments are confusing.
But it should probably be inlined into _loadPartialWord and _storePartialWord and combined with the other bit manipulation for efficiency anyways
There was a problem hiding this comment.
True, removed the comments for now, but agree with inlining it later
There was a problem hiding this comment.
IStore looks good; though I'm not sure how useful MudStoreDeleteRecord is, since it's basically like MudStoreSetRecord with all zeros. There doesn't seem to be a built-in has like with components (which makes sense, since this is generic storage and you usually don't waste a slot on an existence flag)
To summarize about Buffer - imo it currently combines the roles of a static buffer and slice. I'd explore not storing capacity, refactoring it to a Slice, and pairing it with bytes where necessary (and bytes length can be reduced if that's required).
Tried to avoid talking about gas for the most part.
Should I wait for merge before moving to gas optimizations and codegen?
|
First of all, thanks a lot for your review!
There is a subtle difference: with support for callbacks via the
I agree with this, commented above!
I just pushed fixes for the comments marked as resolved, and think we should work on the unresolved ones in follow up PRs. |
Right, makes sense. Later I'd been thinking it might have to do with indexers, hadn't thought about hooks.
Sure, especially with Buffer comments I was expecting to do the changes myself, it overlaps with gas stuff too.
lgtm |
TODO
bytes.concattoabi.encodePackedwhere possible// !gasreportcomments to use themud gas-reportcli commandFollow ups
Bytes,Buffer,Memory- they have some overlap and it’s not entirely clear where the boundaries are right nowLow hanging fruits for gas improvements
pushToDynamicFieldandpopFromDynamicFieldfunctions to store core lib to avoid writing entire array when adding a single element (egCallbackArray)Storage.writeandStorage.readBytes,Buffer,Memory,Storage,SchemaLib,PackedCounterLib)PackedCounterin StoreCore, but gas implications would be interesting. Maybe we can even use abi.encodePacked for data coming out of StoreCore, and have utils to decode them into non-packed with a given schema.