ACL: Remove __acl field

[karim-en]

Currently, the ACL plugins can't be added to already deployed contracts without migration due to the __acl field.
This pull request removes the __acl field and uses the storage key instead.

Added internal/private API:

fn acl_get_storage(&self) -> Option<#acl_type>
fn acl_get_or_init(&mut self) -> #acl_type
fn acl_init_storage_unchecked(&mut self)

[birchmd]

Looks reasonable to me!

[mooori]

Thanks for this change! Initially the acl plugin used near_sdk::collections types which couldn’t be used with near_sdk::env::storage_{read, write}. Great to see it is possible with near_sdk::store types.

---

Why is it necessary to explicitly initiate storage with acl_init_storage()? Couldn’t storage be init'ed implicitly if needed? For instance instead of acl_get there could be acl_get_or_init:

fn acl_get_or_init(&mut self) -> #acl_type {
    // Assume `acl_init_storage_unchecked` returns the struct it wrote to storage.
    self.acl_get_storage().unwrap_or_else(|| self.acl_init_storage_unchecked())
}

This is in line with Acl initiating data structures if needed (example, example) and also other plugins don’t require *_init_storage. Moreover, I think it has the following benefits:

No panics

In the current implementation acl_get may panic and it is called in the implementation of almost every AccessControllable trait method. This means we must explain to users when/why Acl may panic due to storage not being init’ed. With acl_get_or_init this wouldn’t be necessary since there is no panic.

No need to explicitly init storage

Which makes the plugin easier to use.

No need for acl_init_storage in the public API

Since it is init’ed internally if needed.

Note

If going with above proposal, methods that don’t take &mut self cannot call acl_get_or_init(&mut self). Instead they can do something like:

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));
    match self.acl_get_storage() {
        Some(acl) => acl.has_role(role, &account_id),
        None => false
    }
}

[karim-en]

#84 (review)

@mooori @birchmd

Personally, I prefer panic instead of returning default values because normally it shouldn't panic if the contract was implemented correctly, so it indicates there is some bug in the initialization or migration code.

But anyway I've implemented this proposal 35635bc, to be able to merge this pull request asap.

[mooori]

LGTM, just some functions should mutably borrow self, I think (see below).

This sentence is outdated:

https://github.com/aurora-is-near/near-plugins/blob/35635bc6e6e4c9ec7fcf4383ab9d183034809ff0/near-plugins-derive/tests/contracts/access_controllable/src/lib.rs#L124-L125

Possible replacement:

/// Most of them are implemented for the type that holds the plugin's state,
/// which can be accessed with `self.acl_get_or_init()`.

[birchmd]

RE: panic or not to panic

Probably the best solution is to have both functions and let developers decide what makes sense for them. This would be analogous to how Near SDK allows your contract to derive PanicOnDefault or not.

Making panicking on missing storage configurable would be a more complex change so I don't think we should do it as part of this PR, but maybe it is something we want to implement in the future.

[mooori]

Thanks!