refactor!: avoid cloning storage prefixes #92
Conversation
| @@ -25,7 +25,7 @@ pub trait Ownable { | |||
| /// #[ownable(owner_storage_key="CUSTOM_KEY")] | |||
| /// struct Contract { /* ... */} | |||
| /// ``` | |||
| fn owner_storage_key(&self) -> Vec<u8>; | |||
| fn owner_storage_key(&self) -> &'static [u8]; | |||
There was a problem hiding this comment.
I wonder if we should use &[u8] instead of enforcing a static lifetime. I can't really think of any, but maybe there is some use case where the storage key is not known at compile-time for some reason?
There was a problem hiding this comment.
@birchmd Can we merge this as is in order to fix the external issue? I would suggest addressing the change to return &[u8] in a separate issue/PR due to the issue described below.
Returning &[u8] is the better option and it works for all traits except AccessControllable. For the other traits, the function which returns the storage prefix has parameter &self, so returning &[u8] (without lifetime specifier) is fine.
As of now, AccessControllable::acl_storage_prefix cannot take &self due to this usage of Default::default(). Therefore the return value of acl_storage_prefix must have a lifetime specifier. I think we have the following options:
- Refactor the default implementation of
AccessControllableto not rely onDefault. Then return&[u8]from all trait methods that return the storage prefix. - Migrate the other traits to return
&[u8]and acceptAccessControllableas an outlier whose storage prefix function returns&'static [u8]. - Defer this change until a use case pops up where returning
&'static [u8]is not possible.
What do you think?
There was a problem hiding this comment.
Sure, we can merge this as-is. I think doing the refactor of AccessControllable so that all traits can use &[u8] is probably the "right" thing to do, but without a clear use-case it's low priority to actually do it. For I think it's fine to file an issue here on GitHub that this is something we might want to do in the future (or put out a bounty for).
There was a problem hiding this comment.
Tracked by issue #93.
Some functions that return storage prefixes returned
Vec<u8>while&'static [u8]would suffice. This PR changes these functions to return the latter. This avoids unnecessary clones as pointed out in this external issue.Breaking changes
The return type changes from
Vec<u8>to&'static [u8]forOwnable::owner_storage_keyPausable::pa_storage_keyThe bytes which are returned do not change.