where to put runtime permission checks

[4meta5]

If we're using an object in storage to authenticate a runtime method call, then it seems intuitive to pass the AccountId to the method that is doing changes to storage so that it can lookup the object in storage, authenticate the caller, and make the changes with 1 lookup.

Some pseudocode:

decl_moduel!{
   #[weight = 0]
   fn fake_runtime_method(origin, some_arg: u32) -> DispatchResult {
      let caller = ensure_signed(origin)?:
     Self::some_method_to_change_storage(caller, some_arg)?
   }
}

impl<T: Trait> Module<T> {
  fn some_method_to_change_storage(caller: AccountId, some_arg: u32) -> DispatchResult {
     let object_from_storage = <StorageVal<T>>::get();
     ensure!(object_from_storage.authenticate(caller), Error::<T>::CallerCannotChangeStorage);
     let new_object = object_from_storage.update_value(some_arg);
     <StorageVal<T>>::put(new_object);
      Ok(())
   }
}

But passing the AccountId as a parameter limits our ability to call this method from outside this module with different permissions. It seems like the right pattern when we need to constrain the callers to some very specific, static set, but it is the wrong pattern when/if we need to call this logic from outside the module from other logic (like some automated timer that dispatches votes...).

So, an alternative that I suggested is to add Option<AccountId> as the parameter so that the local public runtime methods pass in Some(AccountId), but any methods from outside this module that need to execute this call without these default permissions can pass in None instead. I think it works well for what I need, but it isn't a very ergonomic API pattern.

@dvc94ch suggested adding a call to storage so that authentication happens outside this method but we still constrain the caller. This takes away the AccountId parameter from the method and significantly simplifies things, but it also can add a storage lookup unless we pass the object to the method that is doing changes to storage. If we pass the object to this method, we need a coherent path for outside modules to form the object as well if we want it to be extensible...

It really is a situational preference. I can think of a reason for both patterns but I think having Options as parameters can be generally confusing if the default is not clear and in this case, it might be confusing to communicate that my preference is for methods that default to the bank’s permissions use Some(AccountId) while outside calls could use None when they layer/overwrite permissions