Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: avoid cloning storage prefixes #92

Merged
merged 2 commits into from
Mar 13, 2023
Merged

refactor!: avoid cloning storage prefixes #92

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff8e88b
refactor!: avoid cloning storage prefixes
mooori Mar 13, 2023
8ad8266
Merge branch 'master' into avoid-clones-2
mooori Mar 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions near-plugins-derive/src/ownable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ pub fn derive_ownable(input: TokenStream) -> TokenStream {
let output = quote! {
#[near_bindgen]
impl Ownable for #ident {
fn owner_storage_key(&self) -> Vec<u8> {
(#owner_storage_key).as_bytes().to_vec()
fn owner_storage_key(&self) -> &'static [u8] {
(#owner_storage_key).as_bytes()
}

fn owner_get(&self) -> Option<::near_sdk::AccountId> {
Expand Down
4 changes: 2 additions & 2 deletions near-plugins-derive/src/pausable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ pub fn derive_pausable(input: TokenStream) -> TokenStream {
let output = quote! {
#[near_bindgen]
impl Pausable for #ident {
fn pa_storage_key(&self) -> Vec<u8>{
(#paused_storage_key).as_bytes().to_vec()
fn pa_storage_key(&self) -> &'static [u8] {
(#paused_storage_key).as_bytes()
}

fn pa_is_paused(&self, key: String) -> bool {
Expand Down
2 changes: 1 addition & 1 deletion near-plugins/src/ownable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 AccessControllable to not rely on Default. Then return &[u8] from all trait methods that return the storage prefix.
  • Migrate the other traits to return &[u8] and accept AccessControllable as 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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracked by issue #93.


/// Returns the current owner of the contract. Result must be a NEAR valid account id or None,
/// in case the account doesn't have an owner.
Expand Down
2 changes: 1 addition & 1 deletion near-plugins/src/pausable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ pub trait Pausable {
/// #[pausable(paused_storage_key="CUSTOM_KEY")]
/// struct Contract { /* ... */}
/// ```
fn pa_storage_key(&self) -> Vec<u8>;
fn pa_storage_key(&self) -> &'static [u8];

/// Returns whether feature `key` is paused.
fn pa_is_paused(&self, key: String) -> bool;
Expand Down