feat: add group info to token action screens#263
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces comprehensive support for group-based token actions throughout the backend and UI. Token operation methods and their corresponding UI screens now accept and propagate optional group action context, enabling group authorization, signing, and workflow integration for actions like minting, burning, freezing, and configuration updates. Related data structures are unified and refactored for consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI_Screen
participant AppContext
participant BackendTask
User->>UI_Screen: Selects a token group action
UI_Screen->>AppContext: Prepares IdentityTokenInfo and optional group_info
UI_Screen->>BackendTask: Dispatches token action with group_info
BackendTask->>AppContext: Processes action, integrates group_info
AppContext->>BackendTask: Returns result
BackendTask->>UI_Screen: Updates UI with result
UI_Screen->>User: Displays action outcome
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🔭 Outside diff range comments (4)
src/ui/tokens/freeze_tokens_screen.rs (1)
160-178:⚠️ Potential issuePropagate pre-validation
error_messageinto the structThe value computed above never reaches
self.error_message, hiding useful feedback.- error_message: None, + error_message,src/ui/tokens/resume_tokens_screen.rs (1)
155-173:⚠️ Potential issueInitial validation errors never displayed
error_messagecomputed during constructor is dropped when populating the struct.- error_message: None, + error_message,src/ui/tokens/burn_tokens_screen.rs (1)
164-178:⚠️ Potential issue
error_messagecomputed but thrown away – initialization bug
error_messageis painstakingly built just above (lines 89-154) and later passed toget_selected_wallet, yet the value is discarded here by hard-coding
error_message: None,.
As a result, critical feedback such as “Burning is not allowed on this token” never reaches the UI and the user may wonder why the burn button is disabled.- error_message: None, + error_message,(The surrounding struct field order is unaffected.)
src/ui/tokens/update_token_config.rs (1)
168-194:⚠️ Potential issueSame
error_messageloss as inBurnTokensScreenThe constructor accumulates an
error_message, passes it toget_selected_wallet, and then silently drops it:error_message: None, // unusedUsers will never see permission-related problems.
- error_message: None, // unused + error_message,This mirrors the fix proposed for
BurnTokensScreen.
♻️ Duplicate comments (1)
src/ui/tokens/freeze_tokens_screen.rs (1)
100-118: Misleading error message – says “Burning” instead of “Freezing”Same copy-paste issue as in the destroy screen — error text mismatches the action.
- error_message = Some("Burning is not allowed on this token".to_string()); + error_message = Some("Freezing is not allowed on this token".to_string());Please adjust all branches for clarity.
🧹 Nitpick comments (12)
src/ui/tokens/tokens_screen.rs (1)
1957-2091: The changes follow a consistent pattern of improving error handling across all token actions.All token actions (Burn, Freeze, Destroy, Unfreeze, Pause, Resume, Update Config) have been refactored to follow the same pattern:
- Attempt to convert
IdentityTokenBalancetoIdentityTokenInfousing a context lookup- On success, create the screen with enhanced token data
- On failure, display a meaningful error message
- Close the menu in either case
This consistent approach ensures robust error handling throughout the token management UI, improving the user experience by providing clear feedback when operations cannot proceed due to missing context information.
Consider adding a helper method to reduce code duplication, since the same pattern is repeated for multiple token actions:
+ // Add this helper method to the TokensScreen implementation + fn create_token_action_screen<F>(&mut self, itb: &IdentityTokenBalance, create_screen: F) + where F: FnOnce(IdentityTokenInfo) -> Screen + { + match IdentityTokenInfo::try_from_identity_token_balance_with_lookup(itb, &self.app_context) { + Ok(info) => { + return AppAction::AddScreen(create_screen(info)); + } + Err(e) => { + self.set_error_message(Some(e)); + } + }; + } + + // Then use it like this: + if ui.button("Burn").clicked() { + self.create_token_action_screen(itb, |info| { + Screen::BurnTokensScreen(BurnTokensScreen::new(info, &self.app_context)) + }); + ui.close_menu(); + }src/backend_task/tokens/unfreeze_tokens.rs (1)
40-42: Avoid variable shadowing for clearer intentRe-using the name
group_infoinside theif letshadows the outer parameter.
Although harmless, it slightly hinders readability when stepping through a debugger.-if let Some(group_info) = group_info { - builder = builder.with_using_group_info(group_info); +if let Some(info) = group_info { + builder = builder.with_using_group_info(info); }src/backend_task/tokens/destroy_frozen_funds.rs (1)
40-42: Avoid variable shadowing for clearer intentSame shadowing pattern as in
unfreeze_tokens.rs. Consider renaming the inner binding for consistency and readability (see previous comment).src/ui/tokens/mint_tokens_screen.rs (1)
652-661: Button disabled logic relies on string matchingThe mint button is hidden for non-developers when
button_textcontains"Test":if self.app_context.developer_mode || !button_text.contains("Test") { … }Relying on string inclusion is brittle and breaks if localisation changes the wording.
Prefer an explicit boolean returned byrender_group_action_textsuch asshould_enablealongside the label.let (label, enabled) = render_group_action_text(...); if enabled || self.app_context.developer_mode { … }src/ui/tokens/pause_tokens_screen.rs (2)
96-109: User-facing copy-paste artifacts: “Burning” vs. “Pausing”All error strings still reference “burn”/“burning”, which is misleading on the Pause screen and will bubble up to end-users and logs.
- "Burning is not allowed on this token" + "Pausing is not allowed on this token" … and same for the other three stringsSearch/replace these four occurrences to avoid confusion.
512-518: Brittle button-visibility test
button_text.contains("Test")is a fragile heuristic (e.g. a legitimate token alias containing “Test” would hide the button).
Prefer an explicit boolean fromrender_group_action_text(e.g.should_disable) or inspect the reason instead of substring matching.src/ui/helpers.rs (1)
261-339: UI side-effects mixed with pure value return
render_group_action_textboth mutates the UI (labels/headers) and returns a string for the caller.
Coupling concerns this way:
- Makes the function hard to reuse in contexts where only the text is needed.
- Risks duplicate rendering if accidentally called twice.
Consider splitting into
(a) a purecompute_group_action_text(..) -> (button_label, disable_reason)and
(b) a thin renderer that adds the explanatory UI.src/backend_task/tokens/update_token_config.rs (1)
18-25: PassIdentityTokenInfoby reference to avoid large moves & clones
update_token_configonly reads fromidentity_token_info; passing it by value forces callers to clone large structs and causes an unnecessary move here.- pub async fn update_token_config( - &self, - identity_token_info: IdentityTokenInfo, + pub async fn update_token_config( + &self, + identity_token_info: &IdentityTokenInfo,All subsequent uses become
identity_token_info.field.
This reduces heap copies and keeps the API consistent with other helpers.src/ui/tokens/unfreeze_tokens_screen.rs (1)
101-122: Copy-paste wording: error messages talk about “Burn” instead of “Unfreeze”All branches that set an
error_messagestill use burn-related wording in an unfreeze flow:error_message = Some("Burning is not allowed on this token".to_string()); … "You are not allowed to burn this token"Besides confusing the user, this can complicate telemetry.
Search/replace “burn/Burning” with “unfreeze/Unfreezing”.src/ui/mod.rs (1)
48-50: Unused import after refactor
IdentityTokenBalanceis no longer used insidemod.rsafter migrating token screens toIdentityTokenInfo.
Delete the import to avoid warnings.src/ui/tokens/destroy_frozen_funds_screen.rs (1)
103-129: Copy-paste wording refers to “Burning” instead of “Destroying”All three branches emit an error such as
"Burning is not allowed on this token"even though this screen performs Destroy Frozen Funds.
This is likely to confuse users and bug-hunters.- error_message = Some("Burning is not allowed on this token".to_string()); + error_message = Some("Destroying frozen funds is not allowed on this token".to_string());Please update every affected branch (ContractOwner / Identity / NoOne) in the file.
src/ui/tokens/resume_tokens_screen.rs (1)
95-114: Incorrect terminology in authorisation-failure messagesError strings still reference “Burning” but this screen performs Resume (emergency un-pause). Update for accuracy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
Cargo.toml(1 hunks)src/backend_task/tokens/burn_tokens.rs(3 hunks)src/backend_task/tokens/destroy_frozen_funds.rs(3 hunks)src/backend_task/tokens/freeze_tokens.rs(3 hunks)src/backend_task/tokens/mint_tokens.rs(0 hunks)src/backend_task/tokens/mod.rs(17 hunks)src/backend_task/tokens/pause_tokens.rs(3 hunks)src/backend_task/tokens/resume_tokens.rs(3 hunks)src/backend_task/tokens/transfer_tokens.rs(0 hunks)src/backend_task/tokens/unfreeze_tokens.rs(3 hunks)src/backend_task/tokens/update_token_config.rs(3 hunks)src/database/tokens.rs(2 hunks)src/ui/contracts_documents/group_actions_screen.rs(6 hunks)src/ui/helpers.rs(2 hunks)src/ui/mod.rs(5 hunks)src/ui/tokens/burn_tokens_screen.rs(14 hunks)src/ui/tokens/destroy_frozen_funds_screen.rs(9 hunks)src/ui/tokens/freeze_tokens_screen.rs(9 hunks)src/ui/tokens/mint_tokens_screen.rs(12 hunks)src/ui/tokens/pause_tokens_screen.rs(8 hunks)src/ui/tokens/resume_tokens_screen.rs(8 hunks)src/ui/tokens/tokens_screen.rs(2 hunks)src/ui/tokens/transfer_tokens_screen.rs(1 hunks)src/ui/tokens/unfreeze_tokens_screen.rs(9 hunks)src/ui/tokens/update_token_config.rs(9 hunks)
💤 Files with no reviewable changes (2)
- src/backend_task/tokens/transfer_tokens.rs
- src/backend_task/tokens/mint_tokens.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/backend_task/tokens/update_token_config.rs (2)
src/ui/tokens/update_token_config.rs (2)
ui(884-1043)new(77-194)src/context.rs (1)
new(64-162)
🔇 Additional comments (24)
src/database/tokens.rs (1)
128-133: Accurate documentation for table drop method
The updated doc comment now correctly describes thatdrop_identity_token_balances_tablewill unconditionally drop theidentity_token_balancestable if it exists, matching theDROP TABLE IF EXISTSSQL command. This improves clarity and prevents confusion during schema migrations.src/ui/tokens/transfer_tokens_screen.rs (1)
47-48: Verify field visibility for downstream integration
Exposingreceiver_identity_idandamountaspubaligns this screen’s API with other token action components and allows upstream routers or group‐action flows to initialize these fields. Validation (parsing intoTokenAmount) and error handling remain localized in the UI before dispatching backend tasks.src/ui/tokens/tokens_screen.rs (7)
1957-1972: Code now properly handles potential errors when creating the Burn tokens screen.The code has been improved to handle errors when converting
IdentityTokenBalancetoIdentityTokenInfo. This ensures that if identity or contract information is missing, the user receives an appropriate error message instead of experiencing a runtime failure.
1975-1990: Good error handling implementation for Freeze tokens screen.Similar to the Burn action, the implementation now properly attempts to convert the token balance to token info, providing better error handling and user feedback if the lookup fails to find associated identity or contract information.
1993-2008: Consistent error handling pattern applied to Destroy frozen funds action.The implementation maintains consistency with the same error handling pattern used for other token actions. This systematic approach ensures that users receive clear feedback when required context information isn't available.
2011-2026: Proper conversion and error handling for Unfreeze tokens screen.The implementation correctly attempts to convert token balance information to the richer token info structure, providing appropriate error messages if the conversion fails due to missing identity or contract data.
2029-2044: Enhanced error handling for Pause tokens functionality.The changes maintain the pattern of properly converting token balance to token info before creating the screen, allowing for better error handling and user feedback if the required context data is unavailable.
2047-2062: Resume tokens action now includes proper error handling.The implementation follows the established pattern of attempting to convert token balance to token info with appropriate error handling, ensuring consistent behavior across all token actions.
2076-2091: Update Config action now safely converts token data with error handling.The code now properly attempts to convert token balance to the more comprehensive token info structure before creating the screen, handling potential failures gracefully with user feedback.
src/backend_task/tokens/burn_tokens.rs (3)
7-7: LGTM: Added import for GroupStateTransitionInfoStatusAdding the import for GroupStateTransitionInfoStatus is appropriate for the group context functionality being added to token operations.
25-25: LGTM: Added group_info parameter to support group-based token operationsThe addition of the optional group_info parameter allows token burn operations to be performed within a group context.
40-42: LGTM: Group info handling implementationThe conditional application of group_info to the builder follows good practices by only applying it when present, using the builder pattern consistently.
src/backend_task/tokens/pause_tokens.rs (3)
7-7: LGTM: Added import for GroupStateTransitionInfoStatusAdding the import for GroupStateTransitionInfoStatus is appropriate for the group context functionality being added to token operations.
24-24: LGTM: Added group_info parameter to support group-based token operationsThe addition of the optional group_info parameter allows token pause operations to be performed within a group context.
39-41: LGTM: Group info handling implementationThe conditional application of group_info to the builder follows good practices by only applying it when present, using the builder pattern consistently.
src/backend_task/tokens/resume_tokens.rs (3)
7-7: LGTM: Added import for GroupStateTransitionInfoStatusAdding the import for GroupStateTransitionInfoStatus is appropriate for the group context functionality being added to token operations.
24-24: LGTM: Added group_info parameter to support group-based token operationsThe addition of the optional group_info parameter allows token resume operations to be performed within a group context.
39-41: LGTM: Group info handling implementationThe conditional application of group_info to the builder follows good practices by only applying it when present, using the builder pattern consistently.
src/backend_task/tokens/freeze_tokens.rs (3)
7-7: LGTM: Added import for GroupStateTransitionInfoStatusAdding the import for GroupStateTransitionInfoStatus is appropriate for the group context functionality being added to token operations.
25-25: LGTM: Added group_info parameter to support group-based token operationsThe addition of the optional group_info parameter allows token freeze operations to be performed within a group context.
40-42: LGTM: Group info handling implementationThe conditional application of group_info to the builder follows good practices by only applying it when present, using the builder pattern consistently.
src/ui/tokens/mint_tokens_screen.rs (1)
328-344: Potential mismatch betweengroup_action_idandgroup
group_infois constructed only ifself.groupisSome(_).
Ifgroup_action_idis set butgroupunexpectedly resolved toNone(e.g. corrupted contract), the backend will receiveNoneand silently fall back to a normal mint, which may sign an invalid transition.Recommend adding an explicit sanity check:
if self.group_action_id.is_some() && self.group.is_none() { self.error_message = Some("Group information missing for given action".into()); return; // or early-exit the confirmation path }src/backend_task/tokens/update_token_config.rs (1)
64-69: Redundant clone ofchange_item
change_itemis already owned (TokenConfigurationChangeItemisCopy + Clone).
builder = builder.with_public_note..later borrows it, so cloning here is unnecessary overhead.- change_item.clone(), + change_item,[ suggest_nitpick ]
src/ui/tokens/unfreeze_tokens_screen.rs (1)
300-315:group_infocan beNoneifself.groupisNone– leads to invalid state transitionsWhen
self.group_action_id.is_some()butself.groupisNone(shouldn’t happen but is possible if contract data is malformed),group_infois set toNone, yet the backend task will expect a validGroupStateTransitionInfoStatus.
Return an error early or fall back to a safe default to avoid panics in the task runner.
| BurnTokensScreen::new(identity_token_info, &self.app_context); | ||
|
|
||
| burn_screen.group_action_id = Some(*id); | ||
| burn_screen.amount_to_burn = info |
There was a problem hiding this comment.
This approach with parsing info is a bit hacky and not reliable (if someone will change info format in future, it will break). I would suggest assigning separeate variables, and not mix presentation logic with processing logic.
| if self.group_action_id.is_some() { | ||
| button_text = "Back to Group Actions"; | ||
| } else { | ||
| button_text = "Back to Tokens"; |
There was a problem hiding this comment.
UX note: after updating some token, I expect to go back to that token details screen (with list of identities, its balances, etc), not to list of all tokens.
There was a problem hiding this comment.
Noted. This requires some refactoring of the Tokens Screen which I have been thinking about doing, but out of scope for this PR
| let selected_wallet = | ||
| get_selected_wallet(&identity, None, possible_key.clone(), &mut error_message); | ||
|
|
||
| let group = match identity_token_info |
There was a problem hiding this comment.
it "feels" like the logic to check permissions should be separated to separate function, and maybe made reusable for all action types.
Also applies to other screens.
| KeyType::all_key_types().into(), | ||
| false, | ||
| ); | ||
| pub fn new(identity_token_info: IdentityTokenInfo, app_context: &Arc<AppContext>) -> Self { |
There was a problem hiding this comment.
I wonder if all these similar screens cannot be somewhat generalized - create one function that will be used to build the UI, and just call it here to do like 80% of the job. Now it looks like we are repeating very similar structure over and over. All these screens consist of key selection, group info processing, optional amount input, optional identity selection, optional public note.
3 participants
Change UI on token action screens depending if its a group action. Also route the Group Actions screen to the corresponding group action screen when the Take Action button is clicked.
Summary by CodeRabbit
New Features
Refactor
IdentityTokenBalancewithIdentityTokenInfoacross token action screens and backend logic.Bug Fixes
Documentation