-
Notifications
You must be signed in to change notification settings - Fork 4
Feature/role base access control #112
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.
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
Conversation
and adds a RBAC custom pallet form the template
Permissions map needs to be changed, as they dont vary through scopes, but for roles
and polishes existing functions
Within a helper function
Before updating to the latest offer functionality
…e-base-access-control
sebastianmontero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Pending the pallet_id solution.
…y helper functions calls since we can obtain those parameters from Offers storagemap.
pallets/rbac/src/functions.rs
Outdated
| /// - `scope_id`: The newly generated scope identifier. | ||
| fn create_scope(pallet_id: u64, scope_id: ScopeId)-> DispatchResult{ | ||
| let pallet_id: u64 = pallet_id.try_into().unwrap(); | ||
| fn create_scope(pallet_name: Vec<u8>, scope_id: ScopeId)-> DispatchResult{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to handle the pallet_name/pallet_id parameter as an enum, that can be either the pallet_name or the pallet_id(the hashed pallet name), this enum could have to functions:
- to_id_enum() that returns the enum that contains a pallet_id
- to_id() that returns the actual pallet_id
This would enable: - the encapsulation of all the logic related to this parameter in a type
- the hashing of the name only once in cases where a function that receives this parameter calls multiple other functions that also receives this parameter
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting solution, I developed a similar solution for role name or role id, but I discarded it because it adds some development complexity, I'll try that out again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this implementation look? I'm going to test this enum out
#[derive(Encode, Decode, Clone, Eq, PartialEq,)]
pub enum IdOrVec{
Id([u8;32]),
Vec(Vec<u8>)
}
impl IdOrVec{
pub fn to_id_enum(&self)->Self{
match self{
Self::Id(_) => self.clone(),
Self::Vec(_) => Self::Id(Self::to_id(&self))
}
}
pub fn to_id(&self)->[u8;32]{
match self{
Self::Id(id) => id.clone(),
Self::Vec(v) => v.clone().using_encoded(blake2_256)
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good!
…y coupling instead.
minor clippy optimizations
…eters, I changed the double hashing storagemaps. I removed the tight coupling of timestamp & uniques pallets. I deleted all unused imports. Update Unit tests.
and adds initial setup in RBAC readme
didiermis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.