feat(AccessControllable)!: Remove __acl field (#84)

karim-en · mooori · web-flow · commit 860ec9de9275 · 2023-03-02T07:50:15.000+01:00
* Remove acl field

* Rename acl storage key

* Apply cargo fmt

* Rename `acl_get_field`

* Change gitignore paths

* ACL: don't panic if acl isn't initialized

* Apply suggestions from code review

Co-authored-by: mooori &lt;moritz.zielke@aurora.dev&gt;

* Revert gitignore changes

* Fix comment

---------

Co-authored-by: mooori &lt;moritz.zielke@aurora.dev&gt;
diff --git a/near-plugins-derive/src/access_controllable.rs b/near-plugins-derive/src/access_controllable.rs
@@ -5,7 +5,6 @@ use darling::FromMeta;
 use proc_macro::TokenStream;
 use proc_macro2::Span;
 use quote::quote;
-use syn::parse::Parser;
 use syn::{parse_macro_input, AttributeArgs, ItemFn, ItemStruct};
 
 /// Defines attributes for the `access_controllable` macro.
@@ -17,7 +16,6 @@ pub struct MacroArgs {
 }
 
 const DEFAULT_STORAGE_PREFIX: &str = "__acl";
-const DEFAULT_ACL_FIELD_NAME: &str = "__acl";
 const DEFAULT_ACL_TYPE_NAME: &str = "__Acl";
 
 const ERR_PARSE_BITFLAG: &str = "Value does not correspond to a permission";
@@ -27,13 +25,9 @@ const ERR_PARSE_ROLE: &str = "Value does not correspond to a role";
 pub fn access_controllable(attrs: TokenStream, item: TokenStream) -> TokenStream {
     let cratename = cratename();
     let attr_args = parse_macro_input!(attrs as AttributeArgs);
-    let mut input: ItemStruct = parse_macro_input!(item);
-    let acl_field = syn::Ident::new(DEFAULT_ACL_FIELD_NAME, Span::call_site());
+    let input: ItemStruct = parse_macro_input!(item);
     let acl_type = syn::Ident::new(DEFAULT_ACL_TYPE_NAME, Span::call_site());
     let bitflags_type = new_bitflags_type_ident(Span::call_site());
-    if let Err(e) = inject_acl_field(&mut input, &acl_field, &acl_type) {
-        return TokenStream::from(e.to_compile_error());
-    }
     let ItemStruct { ident, .. } = input.clone();
 
     let macro_args = match MacroArgs::from_list(&attr_args) {
@@ -90,6 +84,7 @@ pub fn access_controllable(attrs: TokenStream, item: TokenStream) -> TokenStream
             Permissions,
             Bearers,
             BearersSet { permission: #bitflags_type },
+            AclStorage,
         }
 
         /// Generates a prefix by concatenating the input parameters.
@@ -100,6 +95,34 @@ pub fn access_controllable(attrs: TokenStream, item: TokenStream) -> TokenStream
             [base, specifier.as_slice()].concat()
         }
 
+        impl #ident {
+            fn acl_get_storage(&self) -> Option<#acl_type> {
+                let base_prefix = <#ident as AccessControllable>::acl_storage_prefix();
+                near_sdk::env::storage_read(&__acl_storage_prefix(
+                    base_prefix,
+                    __AclStorageKey::AclStorage,
+                ))
+                .map(|acl_storage_bytes| {
+                    #acl_type::try_from_slice(&acl_storage_bytes)
+                        .unwrap_or_else(|_| near_sdk::env::panic_str("ACL: invalid acl storage format"))
+                })
+            }
+
+            fn acl_get_or_init(&mut self) -> #acl_type {
+                self.acl_get_storage().unwrap_or_else(|| self.acl_init_storage_unchecked())
+            }
+
+            fn acl_init_storage_unchecked(&mut self) -> #acl_type {
+                let base_prefix = <#ident as AccessControllable>::acl_storage_prefix();
+                let acl_storage: #acl_type = Default::default();
+                near_sdk::env::storage_write(
+                    &__acl_storage_prefix(base_prefix, __AclStorageKey::AclStorage),
+                    &acl_storage.try_to_vec().unwrap(),
+                );
+                acl_storage
+            }
+        }
+
         impl #acl_type {
             fn new_bearers_set(permission: #bitflags_type) -> ::near_sdk::store::UnorderedSet<::near_sdk::AccountId> {
                 let base_prefix = <#ident as AccessControllable>::acl_storage_prefix();
@@ -474,6 +497,39 @@ pub fn access_controllable(attrs: TokenStream, item: TokenStream) -> TokenStream
             }
         }
 
+        fn get_default_permissioned_accounts() -> #cratename::access_controllable::PermissionedAccounts {
+            let roles = <#role_type>::acl_role_variants();
+            let mut map = ::std::collections::HashMap::new();
+            for role in roles {
+                let role: #role_type = ::std::convert::TryFrom::try_from(role)
+                    .unwrap_or_else(|_| ::near_sdk::env::panic_str(#ERR_PARSE_ROLE));
+
+                map.insert(
+                    role.into(),
+                    #cratename::access_controllable::PermissionedAccountsPerRole {
+                        admins: Default::default(),
+                        grantees: Default::default(),
+                    }
+                );
+            }
+
+            #cratename::access_controllable::PermissionedAccounts {
+                super_admins: Default::default(),
+                roles: map,
+            }
+        }
+
+        macro_rules! return_if_none {
+            ($res:expr, $default_value:expr) => {
+                match $res {
+                    Some(val) => val,
+                    None => {
+                        return $default_value;
+                    }
+                }
+            };
+        }
+
         // Note that `#[near-bindgen]` exposes non-public functions in trait
         // implementations. This is [documented] behavior. Therefore some
         // functions are made `#[private]` despite _not_ being public.
@@ -487,56 +543,56 @@ pub fn access_controllable(attrs: TokenStream, item: TokenStream) -> TokenStream
 
             #[private]
             fn acl_init_super_admin(&mut self, account_id: ::near_sdk::AccountId) -> bool {
-                self.#acl_field.init_super_admin(&account_id)
+                self.acl_get_or_init().init_super_admin(&account_id)
             }
 
             fn acl_role_variants(&self) -> Vec<&'static str> {
                 <#role_type>::acl_role_variants()
             }
 
             fn acl_is_super_admin(&self, account_id: ::near_sdk::AccountId) -> bool {
-                self.#acl_field.is_super_admin(&account_id)
+                return_if_none!(self.acl_get_storage(), false).is_super_admin(&account_id)
             }
 
             fn acl_add_admin(&mut self, role: String, account_id: ::near_sdk::AccountId) -> Option<bool> {
                 let role: #role_type = ::std::convert::TryFrom::try_from(role.as_str()).unwrap_or_else(|_| ::near_sdk::env::panic_str(#ERR_PARSE_ROLE));
-                self.#acl_field.add_admin(role, &account_id)
+                self.acl_get_or_init().add_admin(role, &account_id)
             }
 
             fn acl_is_admin(&self, role: String, account_id: ::near_sdk::AccountId) -> bool {
                 let role: #role_type = ::std::convert::TryFrom::try_from(role.as_str()).unwrap_or_else(|_| ::near_sdk::env::panic_str(#ERR_PARSE_ROLE));
-                self.#acl_field.is_admin(role, &account_id)
+                return_if_none!(self.acl_get_storage(), false).is_admin(role, &account_id)
             }
 
             fn acl_revoke_admin(&mut self, role: String, account_id: ::near_sdk::AccountId) -> Option<bool> {
                 let role: #role_type = ::std::convert::TryFrom::try_from(role.as_str()).unwrap_or_else(|_| ::near_sdk::env::panic_str(#ERR_PARSE_ROLE));
-                self.#acl_field.revoke_admin(role, &account_id)
+                self.acl_get_or_init().revoke_admin(role, &account_id)
             }
 
             fn acl_renounce_admin(&mut self, role: String) -> bool {
                 let role: #role_type = ::std::convert::TryFrom::try_from(role.as_str()).unwrap_or_else(|_| ::near_sdk::env::panic_str(#ERR_PARSE_ROLE));
-                self.#acl_field.renounce_admin(role)
+                self.acl_get_or_init().renounce_admin(role)
             }
 
             fn acl_revoke_role(&mut self, role: String, account_id: ::near_sdk::AccountId) -> Option<bool> {
                 let role: #role_type = ::std::convert::TryFrom::try_from(role.as_str()).unwrap_or_else(|_| ::near_sdk::env::panic_str(#ERR_PARSE_ROLE));
-                self.#acl_field.revoke_role(role, &account_id)
+                self.acl_get_or_init().revoke_role(role, &account_id)
             }
 
             fn acl_renounce_role(&mut self, role: String) -> bool {
                 let role: #role_type = ::std::convert::TryFrom::try_from(role.as_str()).unwrap_or_else(|_| ::near_sdk::env::panic_str(#ERR_PARSE_ROLE));
-                self.#acl_field.renounce_role(role)
+                self.acl_get_or_init().renounce_role(role)
             }
 
             fn acl_grant_role(&mut self, role: String, account_id: ::near_sdk::AccountId) -> Option<bool> {
                 let role: #role_type = ::std::convert::TryFrom::try_from(role.as_str()).unwrap_or_else(|_| ::near_sdk::env::panic_str(#ERR_PARSE_ROLE));
-                self.#acl_field.grant_role(role, &account_id)
+                self.acl_get_or_init().grant_role(role, &account_id)
             }
 
 
             fn acl_has_role(&self, role: String, account_id: ::near_sdk::AccountId) -> bool {
                 let role: #role_type = ::std::convert::TryFrom::try_from(role.as_str()).unwrap_or_else(|_| ::near_sdk::env::panic_str(#ERR_PARSE_ROLE));
-                self.#acl_field.has_role(role, &account_id)
+                return_if_none!(self.acl_get_storage(), false).has_role(role, &account_id)
             }
 
             fn acl_has_any_role(&self, roles: Vec<String>, account_id: ::near_sdk::AccountId) -> bool {
@@ -546,61 +602,40 @@ pub fn access_controllable(attrs: TokenStream, item: TokenStream) -> TokenStream
                         ::std::convert::TryFrom::try_from(role.as_str()).unwrap_or_else(|_| ::near_sdk::env::panic_str(#ERR_PARSE_ROLE))
                     })
                     .collect();
-                self.#acl_field.has_any_role(roles, &account_id)
+                return_if_none!(self.acl_get_storage(), false).has_any_role(roles, &account_id)
             }
 
             fn acl_get_super_admins(&self, skip: u64, limit: u64) -> Vec<::near_sdk::AccountId> {
                 let permission = <#bitflags_type>::from_bits(
                     <#role_type>::acl_super_admin_permission()
                 )
                 .unwrap_or_else(|| ::near_sdk::env::panic_str(#ERR_PARSE_BITFLAG));
-                self.#acl_field.get_bearers(permission, skip, limit)
+                return_if_none!(self.acl_get_storage(), vec![]).get_bearers(permission, skip, limit)
             }
 
             fn acl_get_admins(&self, role: String, skip: u64, limit: u64) -> Vec<::near_sdk::AccountId> {
                 let role: #role_type = ::std::convert::TryFrom::try_from(role.as_str()).unwrap_or_else(|_| ::near_sdk::env::panic_str(#ERR_PARSE_ROLE));
                 let permission = <#bitflags_type>::from_bits(role.acl_admin_permission())
                     .unwrap_or_else(|| ::near_sdk::env::panic_str(#ERR_PARSE_BITFLAG));
-                self.#acl_field.get_bearers(permission, skip, limit)
+                return_if_none!(self.acl_get_storage(), vec![]).get_bearers(permission, skip, limit)
             }
 
             fn acl_get_grantees(&self, role: String, skip: u64, limit: u64) -> Vec<::near_sdk::AccountId> {
                 let role: #role_type = ::std::convert::TryFrom::try_from(role.as_str()).unwrap_or_else(|_| ::near_sdk::env::panic_str(#ERR_PARSE_ROLE));
                 let permission = <#bitflags_type>::from_bits(role.acl_permission())
                     .unwrap_or_else(|| ::near_sdk::env::panic_str(#ERR_PARSE_BITFLAG));
-                self.#acl_field.get_bearers(permission, skip, limit)
+                return_if_none!(self.acl_get_storage(), vec![]).get_bearers(permission, skip, limit)
             }
 
             fn acl_get_permissioned_accounts(&self) -> #cratename::access_controllable::PermissionedAccounts {
-                self.#acl_field.get_permissioned_accounts()
+                return_if_none!(self.acl_get_storage(), get_default_permissioned_accounts()).get_permissioned_accounts()
             }
         }
     };
 
     output.into()
 }
 
-fn inject_acl_field(
-    item: &mut ItemStruct,
-    field_name: &syn::Ident,
-    acl_type: &syn::Ident,
-) -> Result<(), syn::Error> {
-    let fields = match item.fields {
-        syn::Fields::Named(ref mut fields) => fields,
-        _ => {
-            return Err(syn::Error::new(
-                item.ident.span(),
-                "Expected to have named fields",
-            ))
-        }
-    };
-
-    fields.named.push(syn::Field::parse_named.parse2(quote! {
-        #field_name: #acl_type
-    })?);
-    Ok(())
-}
-
 /// Defines attributes for the `access_control_any` macro.
 #[derive(Debug, FromMeta)]
 pub struct MacroArgsAny {
diff --git a/near-plugins-derive/tests/contracts/access_controllable/src/lib.rs b/near-plugins-derive/tests/contracts/access_controllable/src/lib.rs
@@ -37,11 +37,7 @@ impl Counter {
     /// identifier of the enum variant, i.e. `"Updater"` for `Role::Updater`.
     #[init]
     pub fn new(admins: HashMap<String, AccountId>, grantees: HashMap<String, AccountId>) -> Self {
-        let mut contract = Self {
-            counter: 0,
-            // Initialize `AccessControllable` plugin state.
-            __acl: Default::default(),
-        };
+        let mut contract = Self { counter: 0 };
 
         if admins.len() > 0 || grantees.len() > 0 {
             // First we make the contract itself super admin to allow it adding admin and grantees.
@@ -68,11 +64,11 @@ impl Counter {
             // granting roles, for example:
             //
             // ```
-            // contract.__acl.add_admin_unchecked(role, account_id);
-            // contract.__acl.grant_role_unchecked(role, account_id);
+            // contract.acl_get_or_init().add_admin_unchecked(role, account_id);
+            // contract.acl_get_or_init().grant_role_unchecked(role, account_id);
             // ```
             //
-            // **Attention**: for security reasons, `__acl.*_unchecked` methods should only be called
+            // **Attention**: for security reasons, `acl_get_or_init().*_unchecked` methods should only be called
             // from within methods with attribute `#[init]` or `#[private]`.
         }
 
@@ -126,7 +122,7 @@ impl Counter {
     /// The implementation of `AccessControllable` provided by `near-plugins`
     /// adds further methods to the contract which are not part of the trait.
     /// Most of them are implemented for the type that holds the plugin's state,
-    /// here `__acl`.
+    /// which can be accessed with `self.acl_get_or_init()`.
     ///
     /// This function shows how these methods can be exposed on the contract.
     /// Usually this should involve security checks, for example requiring the
@@ -136,7 +132,7 @@ impl Counter {
             self.acl_is_super_admin(env::predecessor_account_id()),
             "Only super admins are allowed to add other super admins."
         );
-        self.__acl.add_super_admin_unchecked(&account_id)
+        self.acl_get_or_init().add_super_admin_unchecked(&account_id)
     }
 }
 
@@ -145,31 +141,32 @@ impl Counter {
 impl Counter {
     #[private]
     pub fn acl_add_super_admin_unchecked(&mut self, account_id: AccountId) -> bool {
-        self.__acl.add_super_admin_unchecked(&account_id)
+        self.acl_get_or_init().add_super_admin_unchecked(&account_id)
     }
 
     #[private]
     pub fn acl_revoke_super_admin_unchecked(&mut self, account_id: AccountId) -> bool {
-        self.__acl.revoke_super_admin_unchecked(&account_id)
+        self.acl_get_or_init().revoke_super_admin_unchecked(&account_id)
     }
 
     #[private]
     pub fn acl_revoke_role_unchecked(&mut self, role: Role, account_id: AccountId) -> bool {
-        self.__acl.revoke_role_unchecked(role.into(), &account_id)
+        self.acl_get_or_init()
+            .revoke_role_unchecked(role.into(), &account_id)
     }
 
     #[private]
     pub fn acl_add_admin_unchecked(&mut self, role: Role, account_id: AccountId) -> bool {
-        self.__acl.add_admin_unchecked(role, &account_id)
+        self.acl_get_or_init().add_admin_unchecked(role, &account_id)
     }
 
     #[private]
     pub fn acl_revoke_admin_unchecked(&mut self, role: Role, account_id: AccountId) -> bool {
-        self.__acl.revoke_admin_unchecked(role, &account_id)
+        self.acl_get_or_init().revoke_admin_unchecked(role, &account_id)
     }
 
     #[private]
     pub fn acl_grant_role_unchecked(&mut self, role: Role, account_id: AccountId) -> bool {
-        self.__acl.grant_role_unchecked(role, &account_id)
+        self.acl_get_or_init().grant_role_unchecked(role, &account_id)
     }
 }
diff --git a/near-plugins-derive/tests/contracts/pausable/src/lib.rs b/near-plugins-derive/tests/contracts/pausable/src/lib.rs
@@ -40,7 +40,6 @@ impl Counter {
     pub fn new(pause_manager: AccountId) -> Self {
         let mut contract = Self {
             counter: 0,
-            __acl: Default::default(),
         };
 
         // Make the contract itself super admin. This allows us to grant any role in the
