fix: underway votes type #1625
fix: underway votes type #1625
Conversation
WalkthroughThe pull request introduces several enhancements to the governance components within the application. Key changes include the creation of a new tooltip component for displaying locked amounts, refactoring of the Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/extension-polkagate/src/fullscreen/governance/delegate/partial/AmountWithOptionsAndLockAmount.tsx (2)
41-41: Remove debug console.log statement.Debug logging should be removed before merging to production.
- console.log('accountLocks :::', accountLocks);
131-131: Remove debug console.log statement.Debug logging should be removed before merging to production.
- console.log('accountLocks:', accountLocks);packages/extension-polkagate/src/hooks/useAccountLocks.ts (1)
58-59: Consider adding tests for lock conviction scenarios.To prevent future regressions and ensure correct handling of locked amounts, consider adding test cases that verify the behavior for different voting scenarios (approved, rejected, ongoing) and their corresponding conviction types.
Would you like me to help generate comprehensive test cases for the
getLocksfunction?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/extension-polkagate/src/fullscreen/governance/delegate/partial/AmountWithOptionsAndLockAmount.tsx(4 hunks)packages/extension-polkagate/src/fullscreen/governance/post/myVote/index.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/governance/post/myVote/util.tsx(2 hunks)packages/extension-polkagate/src/hooks/useAccountLocks.ts(1 hunks)
🔇 Additional comments (8)
packages/extension-polkagate/src/fullscreen/governance/post/myVote/index.tsx (2)
65-65: LGTM: Improved vertical alignment
The addition of alignItems='center' to the Grid container improves the vertical alignment consistency of the balance and conviction information.
68-68: Verify the type casting of voteBalance
The casting of voteBalance through unknown to BN type seems like a workaround. This could potentially mask type-related issues.
Let's verify the type definitions and usage:
packages/extension-polkagate/src/fullscreen/governance/post/myVote/util.tsx (2)
69-69: Please clarify the TODO comment and provide proper typing.
The comment "needs to be fixed" suggests this is a temporary solution. Given that this PR aims to fix locked amount information, we should:
- Determine the correct type for
delegating - Update the type accordingly
- Remove the TODO comment
The commented-out code below suggests there's more work needed here.
Let's analyze the usage to determine the correct type:
#!/bin/bash
# Search for the structure of delegating data to determine proper typing
ast-grep --pattern 'delegating: $_'Would you like help determining the correct type for the delegating property?
63-63: LGTM! Verify usage of prior property.
The type change from any to unknown improves type safety. This is a good change that enforces proper type checking.
Let's verify the impact of this type change:
✅ Verification successful
Let me analyze the usage patterns to verify the impact of changing prior from any to unknown.
Based on the search results, I can see that the prior property is used in the Vote interface in packages/extension-polkagate/src/fullscreen/governance/post/myVote/util.tsx and is primarily used in the governance voting context. The type change from any to unknown is a type-safety improvement that doesn't appear to cause any issues, as the property is mostly used for type checking and passing through the voting system without direct manipulation of its values.
Type change to unknown is safe and properly implemented.
The codebase shows that the prior property is accessed in a type-safe manner through the governance voting system, and the change to unknown aligns with TypeScript best practices without breaking existing functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of the prior property to ensure they handle the unknown type correctly
rg -A 3 "\.prior\b"
Length of output: 131
Script:
#!/bin/bash
# Search for usages of prior property with more context and case-insensitive
rg -i "prior" -A 5 --type typescript
# Also search for Vote interface usage
ast-grep --pattern 'Vote {
$$$
}'
# Search for Vote type references
rg "Vote[\[\]\s{}<>]" --type typescript
Length of output: 186
Script:
#!/bin/bash
# Search for usages of prior property with more context
rg -i "prior" -A 5
# Also search for Vote interface usage and type references
rg "Vote[\[\]\s{}<>]"
# Find TypeScript/JavaScript files that might use the Vote interface
fd "\.(ts|tsx|js|jsx)$" --exec grep -l "Vote"
Length of output: 42496
packages/extension-polkagate/src/fullscreen/governance/delegate/partial/AmountWithOptionsAndLockAmount.tsx (3)
33-38: LGTM! Well-structured type definition.
The AlreadyLockedTooltipTextProps interface clearly defines the required props for the tooltip component.
Line range hint 128-157: LGTM! Well-structured component implementation.
The component properly handles props and maintains good separation of concerns. The use of translation and child components is well organized.
40-80: Verify the locked amount calculation logic.
The new AlreadyLockedTooltipText component provides detailed information about locked amounts, which should help address the core issue. Let's verify the calculation logic:
✅ Verification successful
Based on the code analysis, I can now generate the final response:
The locked amount calculation and display implementation is correct and properly handles all cases.
The implementation correctly:
- Uses
getAlreadyLockedValueto calculate total locked amount by sorting locks by amount and type - Properly displays lock details including reference IDs, amounts, multipliers, and expiry times
- Handles edge cases like 'None' locks and ongoing locks
- Correctly formats amounts using
amountToHumanwith proper decimal places - Maintains type safety through proper TypeScript interfaces
The tooltip implementation aligns with the PR's objective of fixing wrong locked amount information by providing clear, detailed breakdowns of all locks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related locked amount calculations
rg -A 5 "getLocks|lockedAmount|votingBalance" --type typescript
Length of output: 100
Script:
#!/bin/bash
# Search for related locked amount calculations without file type restriction
rg -A 5 "getLocks|lockedAmount|votingBalance"
# Also search for specific lock-related functions
ast-grep --pattern 'function $_(locks$_) {
$$$
}'
Length of output: 37447
Script:
#!/bin/bash
# Search for lock-related functions and their implementations
ast-grep --pattern 'function getAlreadyLockedValue($_) {
$$$
}'
# Also search for lock-related imports and constants
rg "import.*Lock|const.*Lock" -A 2
Length of output: 42111
Script:
#!/bin/bash
# Search for getAlreadyLockedValue implementation
rg -A 10 "getAlreadyLockedValue" --no-filename
# Search for lock amount calculation logic
rg -A 5 "total.*lock|lock.*total" --no-filename
Length of output: 12227
packages/extension-polkagate/src/hooks/useAccountLocks.ts (1)
58-59: Verify the impact of unconditional conviction type assignment.
The change to always set locked = vote.conviction.type regardless of vote outcome looks correct, as it better reflects the actual conviction type chosen by the voter. This should help fix the wrong locked amount information issue mentioned in #1562.
Let's verify this doesn't affect other parts of the codebase:
✅ Verification successful
Based on the gathered information, I can now generate the final response:
The change to unconditionally set conviction type is correct and safe.
The code analysis reveals that:
- The conviction type is only used for UI display purposes, showing lock information in delegate-related components
- The UI components correctly handle and display the conviction-based locks (e.g., in
SupportUs.tsx: "tokens will be temporarily locked based on your chosen conviction level") - The
useTimeToUnlockhook, which depends on these locks, works with the total amounts and end blocks, not the conviction type itself - No test files were found that verify this behavior, suggesting it's primarily a UI-focused change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any UI components that might depend on the locked value
rg -l "locked.*conviction" --type ts --type tsx
# Search for test files that verify lock behavior
rg -l "getLocks.*conviction" --type test
Length of output: 149
Script:
#!/bin/bash
# Search for any UI components that might depend on the locked value, using proper file extensions
rg -l "locked.*conviction" -g '*.ts' -g '*.tsx'
# Search for test files that verify lock behavior
rg -l "getLocks.*conviction" -g '*.test.ts' -g '*.spec.ts'
# Let's also check for any direct usage of useAccountLocks hook
rg -l "useAccountLocks" -g '*.ts' -g '*.tsx'
# And look for any similar conviction type assignments
ast-grep --pattern 'locked = $_.conviction.type'
Length of output: 1743
Script:
#!/bin/bash
# Let's check the context of how locked conviction is used in the UI components
rg "locked.*conviction" -A 5 -B 5 packages/extension-polkagate/src/fullscreen/governance/old-SupportUs.tsx packages/extension-polkagate/src/fullscreen/governance/SupportUs.tsx
# Let's also check how the useAccountLocks hook is used in delegate-related components
rg "useAccountLocks" -A 3 -B 3 packages/extension-polkagate/src/fullscreen/governance/delegate/
# And check the implementation of useTimeToUnlock which might depend on this behavior
cat packages/extension-polkagate/src/hooks/useTimeToUnlock.tsx
Length of output: 17057
packages/extension-polkagate/src/fullscreen/governance/post/myVote/util.tsx
Show resolved
Hide resolved
...sion-polkagate/src/fullscreen/governance/delegate/partial/AmountWithOptionsAndLockAmount.tsx
Outdated
Show resolved
Hide resolved
fix: remove logs
AMIRKHANEF
force-pushed
the
fixUnderwayVotesType
branch
from
b19e8c0 to
84e7c9a
Compare
## [0.21.5](v0.21.4...v0.21.5) (2024-11-03) ### Bug Fixes * underway votes type ([#1625](#1625)) ([42be4e2](42be4e2))
Close: #1562
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
getLocksfunction for consistent handling of locked variables.