Isolate Role Authorization #24
Conversation
| if self.has_role(store, role, caller)? { | ||
| // if any exists, stop iteration and return true result | ||
| return Ok(true) | ||
| } else { |
There was a problem hiding this comment.
Fixed in 8da9b7f 👍
| @@ -0,0 +1,61 @@ | |||
| version: 2.1 | |||
There was a problem hiding this comment.
I think we should move the CircleCI config to the top level of the repo. Also don't think it's that important to have the gitpod stuff in the CI. I think bare minimum we should have lint, test, build CI checks for the contracts + packages. Something similar to the way it's setup in DAO DAO and cw-plus
There was a problem hiding this comment.
The DAODAO workflows would be good. This is all boilerplate from the Interwasm cw template
| @@ -11,22 +11,9 @@ pub struct InstantiateMsg { | |||
| #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)] | |||
There was a problem hiding this comment.
What's the rationale for having all of the messages defined in the ultra-base package? Having these defined separately from the main contract hinders readability a bit. IMO only shared types and code should be in the packages, everything else can be defined directly in the contracts.
There was a problem hiding this comment.
I agree. Let's tackle this next. Opened an issue #25
| @@ -23,6 +16,17 @@ pub struct SudoParams { | |||
| pub owner: Addr, | |||
| } | |||
|
|
|||
| pub struct State<'a> { | |||
There was a problem hiding this comment.
Hmm... I'm a bit confused about the usage of State. It needs the role provider address to instantiate state.roles (which has type RoleConsumer). But everywhere it's used, you're calling State::default which passes in a dummy "role_provider_address".
Shouldn't we be doing the following instead:
- Store State as an item
pub const STATE: Item<State> = Item::new("state");
- Create a new State object with the actual role_provider address in contract instantiation
let role_provider = deps.api.addr_validate(&msg.role_provider)?;
let state = State {
roles: RoleConsumer::new(role_provider),
};
STATE.save(deps.storage, &state)?;
TBH I'm a bit confused in general about the RoleConsumer usage. Wondering if there's a simpler way to accomplish the same thing
There was a problem hiding this comment.
@vernonjohnson State is a struct of storage objects that manages contract state. This pattern makes it easier to implement lifetimes for storage types.
RoleConsumer is struct that wraps Item::new("role_provider_address") and provides common functionality to query the role provider contract via its given address.
|
Triple checked that all role requirements were migrated accurately. Merging. |
Currently role authorization is redundant and stored locally across every contract:
This PR isolates role storage to a single contract and implements a Controller/Provider pattern for easy use and isolated testing.