feat: storage layout + read/write slots + mut keyword #30
Conversation
| name: String, | ||
| symbol: String, | ||
| decimals: u8, | ||
| total_supply: Slot<U256>, |
There was a problem hiding this comment.
We chatted about some of this offline, I think it's a bit odd to have Slot here but not in Mapping, but good for a first version and can be improved in the future.
There was a problem hiding this comment.
So if I understand correctly, every type used in a struct that derives storage has to implement StorageLayout, right?
There was a problem hiding this comment.
We chatted about some of this offline, I think it's a bit odd to have
Slothere but not in Mapping, but good for a first version and can be improved in the future.
yes, my bad. I totally forgot... I'll amend the PR to incorporate that design pattern.
So if I understand correctly, every type used in a struct that derives
storagehas to implementStorageLayout, right?
yes, that's correct they need to impl StorageLayout so that we can ensure that we can allocate a slot in the layout for them.
There was a problem hiding this comment.
I think this is fine as is and we can iterate later on
| pub fn total_supply(&self) -> U256 { | ||
| self.total_supply | ||
| self.total_supply.read() |
There was a problem hiding this comment.
I think you wrote this somewhere, but pub fields could also get automatic getters, can be done in the future. (let's open an issue)
| fn call(&self); | ||
| fn call_with_data(&self, calldata: &[u8]); | ||
| fn call(&mut self); | ||
| fn call_with_data(&mut self, calldata: &[u8]); |
There was a problem hiding this comment.
It could also make sense later on to have immutable versions of these, not sure how that would work but it'd be cool.
There was a problem hiding this comment.
agreed.
do you foresee any scenario other than static calls where we would use the not mutable call?
| padded[..bytes.len()].copy_from_slice(&bytes); | ||
| sstore(key, U256::from_be_bytes(padded)); | ||
| impl<K, V> StorageLayout for Mapping<K, V> { | ||
| fn allocate(slot: u64) -> Self { |
| /// Wrapper around `alloy::primitives` that can be written in a single slot (single EVM word). | ||
| #[derive(Default)] | ||
| pub struct Slot<V> { | ||
| id: u64, |
There was a problem hiding this comment.
Technically, shouldn't the id be u256? I agree it should be fine as u64 if we incrementally allocate slots, but to be fully compatible/flexible and allow custom storage layouts from the user I think this would have to be u256?
There was a problem hiding this comment.
yes, it should be u256, i just kept the u64 as that's what was being used in Mapping. I'll do a follow-up commit to make everything compliant.
| }); | ||
|
|
||
| // Generate initialization code for each field | ||
| let init_fields = fields.iter().enumerate().map(|(slot, f)| { |
There was a problem hiding this comment.
I think this is fine for now, but we should spend some time crafting how storage layout will work. For example, this won't allow for storing a (u256, u256) sequentially, we'd need to implement StorageLayout for it with a phantom slot, and then in the actual read/write use some encoded key (similar to Mapping). Ideally we'd know the storage size of the type at compile time (if value type) and use that size for allocation instead of enumerate. Reference types (Mapping, arrays, etc) could work like Mapping. This is what Solidity does also.
There was a problem hiding this comment.
This is a really good point and i totally missed that. Do you want me to try to address it in this PR? or would u open an issue and do it in the following one?
There was a problem hiding this comment.
I think later is fine
mutkeyword for write operations, so that the compiler complains when a contract fn tries to update storage without&mut selfSlot<V>for primitives that fit a single storage slot (equivalent to the oldWord<V>>>> as discussed, we don't like this wrapper type, so i think that down the line we can enhance thecontractmacro so that the devex is like working with primitives.storagethat initializes the storage layout >>> it requires all the attributes of the struct to implement theStorageLayouttrait (some notes regarding this point):StorageLayouttrait is unnecessary and it could be merged with the existingStorageStorable.u64toOption<u64>or something like this.PS: i know i'm not the best at coming up with good names, so feel free to propose new ones