Conversation
austbot
left a comment
austbot
left a comment
There was a problem hiding this comment.
Looks really good just some questions on perf and reusability of code and some of the iterations you are doing
| let mut cursor = 0; | ||
| let end_pos = role.actions.len(); | ||
|
|
||
| while cursor < end_pos { |
There was a problem hiding this comment.
why this while loop nonsense instead of Role get action or rolueMut::get_Action_mut
There was a problem hiding this comment.
totally forgot about match_data / get_action_mut. updated here: 0d781c7#diff-271f9fd294483735c02257851e36ba7f48044df12711a853368a45838c332737R285
| )?; | ||
|
|
||
| // Find the SubAccount action for this specific index | ||
| // We need custom logic to match by index since multiple SubAccount actions may |
There was a problem hiding this comment.
why does this matter, does a sub account have permissions now?
I think as long as the role ha ssub account you can terminate at the first one.
Ah i see what you are doing
There was a problem hiding this comment.
I think it may behove us to make action interface also have a match fn so we can iterate with predicate
There was a problem hiding this comment.
Updated the match_data impl to allow for matching based on a specific index: 0d781c7#diff-51332229da15082ffb7cbdff13008826a473567758857466f5c4d53244a42e56R119
| /// | ||
| /// # Returns | ||
| /// * `Result<(), ProgramError>` - Ok if no duplicates, error otherwise | ||
| fn validate_no_duplicate_sub_account_indices(actions_data: &[u8]) -> Result<(), ProgramError> { |
There was a problem hiding this comment.
should this be used in create aswell?
There was a problem hiding this comment.
This is already in create_v1 via swig_builder.add_role() here: https://github.com/anagrambuild/swig-wallet/pull/124/files#diff-7cdee2a0ab3a5cb8184913ac9b3d863aea8a47a87bae3375ec73e9519a425c7cR433-R463
| @@ -189,29 +200,97 @@ pub fn withdraw_from_sub_account_v1( | |||
| if !action.enabled { | |||
There was a problem hiding this comment.
seems like we should be able to disable and then withdraw as the authority
There was a problem hiding this comment.
updated to allow for withdraw from subaccount when disabled: 0d781c7#diff-64d13480044b74ce5b687ca18234ae948b058d8ed1fc657f72bd8279c5b9410bR191-R195
| ); | ||
| return Err(SwigAuthenticateError::PermissionDeniedMissingPermission.into()); | ||
| } else if all_action.is_some() { | ||
| // All permission: Search all roles to find which one has the SubAccount action |
There was a problem hiding this comment.
why do we need to iterate over all roles not just the current chekcing for subaccount and manage or all
There was a problem hiding this comment.
We need to get the subaccount's index since it's part of the signer seeds, which is stored on the SubAccount action. We could add it as an optional arg where it defaults to 0 unless specified otherwise. let me know what you think
2 participants
problem
You can only have one SubAccount per role
solution
Use one byte of padding on the SubAccount to allow for setting a specific
indexthat can be used as part of the PDA seed so one role can have multiple subaccounts.Typescript sdk updates:anagrambuild/swig-ts#85
Docs updates: anagrambuild/swig-docs#16