fix: Move execution of fee estimation within the Tx bounds #332
Conversation
This allows for isSigning to be set right away, before proceeding to estimating fees, which can take a while. Thus every component reacting to isSigning (and showing the three bouncing dots) will be disabled right away, without waiting for server requests to finish. Also moved the validation schema for sendForm to its own schema file and import. I did not want to touch ibcSendForm, but the idea would be to reuse the schema there too. Will do in a follow up PR. Finally, fixed some internals with AmountInput and the way it interacts with schemas and Formik. This new version returns only actual number (or undefined) instead of a string that needs parsing. This makes it easier to use in most cases too. Fixes liftedinit#308
WalkthroughThis pull request refactors fee estimation across multiple components by replacing synchronous fee calculations with deferred callback functions. Now, instead of awaiting the result of an immediate fee computation, components pass a function that calls Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Component/UI
participant TX as Transaction Handler (tx)
participant FEE as Fee Callback (estimateFee)
UI->>TX: Invoke transaction with fee callback
TX->>FEE: Call fee callback to estimate fee
FEE-->>TX: Return computed fee
TX->>TX: Process transaction using the returned fee
TX->>UI: Trigger onSuccess callback
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #332 +/- ##
==========================================
+ Coverage 53.00% 53.18% +0.18%
==========================================
Files 220 221 +1
Lines 16927 16890 -37
==========================================
+ Hits 8972 8983 +11
+ Misses 7955 7907 -48 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
schemas/sendForm.ts (2)
25-26: Address the TODO comment for improved type safety.The comment indicates a need to use the proper type
CombinedBalanceInfofor selectedToken instead ofany.- // TODO: Use the proper type (CombinedBalanceInfo) for selectedToken. - selectedToken: Yup.mixed<any>().nullable().required('Please select a token'), + selectedToken: Yup.mixed<CombinedBalanceInfo>().nullable().required('Please select a token'),
27-43: Consider making MIN_FEE_BUFFER configurable.The validation uses a hard-coded value of 0.09 for the minimum fee buffer. To improve maintainability, consider extracting this as a named constant at the file or module level.
+ const MIN_FEE_BUFFER = 0.09; + export const schema = Yup.object().shape({ // ... other schema fields feeWarning: Yup.mixed() .nullable() .test('leave-for-fees', 'Please confirm you understand the fee', function () { // ... other code - const MIN_FEE_BUFFER = 0.09; const hasInsufficientBuffer = amount > balance - MIN_FEE_BUFFER; // ... rest of functioncomponents/react/inputs/AmountInput.tsx (2)
34-38: Consider clarifying support for exponential notation.Currently, the regex in line 41 blocks non-integer and non-decimal input except for a dot. Yet later test cases show inputs like "1E+10," which do not match the regex and revert to the last valid value. If exponential notation is intended to be supported, consider updating the regex and/or logic accordingly. Otherwise, remember that typing "e/E" in the UI is blocked by
onKeyDown.- const newValue = /^\d*\.?\d*$/.test(v) ? parseFloat(v) : NaN; + // Optionally allow for exponential notation if desired: + const newValue = /^[0-9]*\.?[0-9]*(e[+-]?[0-9]+)?$/i.test(v) ? parseFloat(v) : NaN;Also applies to: 41-47
61-63: Add a test for theonKeyDownlogic.Since lines 61-63 are not covered by tests, consider adding a test case that simulates user keyboard input to confirm that non-numeric keys (e.g., letters) are prevented.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 61-63: components/react/inputs/AmountInput.tsx#L61-L63
Added lines #L61 - L63 were not covered by testscomponents/react/inputs/__tests__/AmountInput.test.tsx (1)
47-48: Validate mismatch between user-typing and test scenario.The tests confirm that typing invalid numeric strings (e.g., "42.42.42") reverts to either the last valid value or
undefined. However, certain test inputs (like "1E+10") cannot be typed at runtime due to the keydown filter. These tests rely onfireEvent.change, which bypassesonKeyDown. If the actual use-case needs exponent notation, re-check the regex or remove those test scenarios if they’re purely artificial.Also applies to: 62-63, 77-78, 92-93, 95-96, 98-99, 113-114
components/bank/forms/sendForm.tsx (2)
66-70: Consider unit tests for transaction flow.The
handleSendfunction (lines 66-70) includes parsing amounts and constructing theMsgSendor group proposal message. Consider covering these paths with tests to verify that:
values.amountis correctly converted into base units.- The fallback path for group proposals triggers as expected.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 66-66: components/bank/forms/sendForm.tsx#L66
Added line #L66 was not covered by tests
[warning] 69-69: components/bank/forms/sendForm.tsx#L69
Added line #L69 was not covered by tests
122-127: Defaultingamountto zero.Defaulting
amountto0in theinitialValuesis fine if the design expects a default of zero. If you prefer leaving it empty, consider setting it toundefinedand letting the schema handle the required checks. This might more clearly convey an “uninitialized” state for the user.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
components/admins/modals/cancelUpgradeModal.tsx(1 hunks)components/admins/modals/multiMfxBurnModal.tsx(1 hunks)components/admins/modals/multiMfxMintModal.tsx(1 hunks)components/admins/modals/upgradeModal.tsx(1 hunks)components/admins/modals/validatorModal.tsx(1 hunks)components/admins/modals/warningModal.tsx(1 hunks)components/bank/forms/ibcSendForm.tsx(8 hunks)components/bank/forms/sendForm.tsx(7 hunks)components/factory/forms/BurnForm.tsx(4 hunks)components/factory/forms/ConfirmationForm.tsx(2 hunks)components/factory/forms/MintForm.tsx(1 hunks)components/factory/modals/TransferModal.tsx(1 hunks)components/factory/modals/updateDenomMetadata.tsx(1 hunks)components/groups/forms/groups/ConfirmationForm.tsx(1 hunks)components/groups/modals/groupInfo.tsx(1 hunks)components/groups/modals/memberManagementModal.tsx(1 hunks)components/groups/modals/updateGroupModal.tsx(1 hunks)components/groups/modals/voteDetailsModal.tsx(3 hunks)components/react/inputs/AmountInput.tsx(3 hunks)components/react/inputs/__tests__/AmountInput.test.tsx(6 hunks)hooks/useTx.tsx(3 hunks)schemas/index.ts(1 hunks)schemas/sendForm.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
components/bank/forms/ibcSendForm.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/bank/forms/ibcSendForm.tsx:149-152
Timestamp: 2024-11-12T13:03:18.927Z
Learning: In `components/bank/forms/ibcSendForm.tsx`, the `handleSend` function relies on the `useTx` hook for error handling, so additional error handling or user notifications within `handleSend` are unnecessary.
components/bank/forms/sendForm.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/bank/forms/ibcSendForm.tsx:149-152
Timestamp: 2024-11-12T13:03:18.927Z
Learning: In `components/bank/forms/ibcSendForm.tsx`, the `handleSend` function relies on the `useTx` hook for error handling, so additional error handling or user notifications within `handleSend` are unnecessary.
🪛 GitHub Check: codecov/patch
components/factory/modals/TransferModal.tsx
[warning] 88-88: components/factory/modals/TransferModal.tsx#L88
Added line #L88 was not covered by tests
components/admins/modals/warningModal.tsx
[warning] 68-68: components/admins/modals/warningModal.tsx#L68
Added line #L68 was not covered by tests
components/factory/forms/MintForm.tsx
[warning] 93-93: components/factory/forms/MintForm.tsx#L93
Added line #L93 was not covered by tests
components/groups/forms/groups/ConfirmationForm.tsx
[warning] 87-87: components/groups/forms/groups/ConfirmationForm.tsx#L87
Added line #L87 was not covered by tests
components/admins/modals/cancelUpgradeModal.tsx
[warning] 68-68: components/admins/modals/cancelUpgradeModal.tsx#L68
Added line #L68 was not covered by tests
components/admins/modals/multiMfxMintModal.tsx
[warning] 103-103: components/admins/modals/multiMfxMintModal.tsx#L103
Added line #L103 was not covered by tests
components/groups/modals/groupInfo.tsx
[warning] 143-143: components/groups/modals/groupInfo.tsx#L143
Added line #L143 was not covered by tests
components/groups/modals/memberManagementModal.tsx
[warning] 162-162: components/groups/modals/memberManagementModal.tsx#L162
Added line #L162 was not covered by tests
components/admins/modals/multiMfxBurnModal.tsx
[warning] 100-100: components/admins/modals/multiMfxBurnModal.tsx#L100
Added line #L100 was not covered by tests
components/factory/modals/updateDenomMetadata.tsx
[warning] 129-129: components/factory/modals/updateDenomMetadata.tsx#L129
Added line #L129 was not covered by tests
components/admins/modals/upgradeModal.tsx
[warning] 155-155: components/admins/modals/upgradeModal.tsx#L155
Added line #L155 was not covered by tests
components/groups/modals/voteDetailsModal.tsx
[warning] 125-125: components/groups/modals/voteDetailsModal.tsx#L125
Added line #L125 was not covered by tests
[warning] 142-142: components/groups/modals/voteDetailsModal.tsx#L142
Added line #L142 was not covered by tests
[warning] 163-163: components/groups/modals/voteDetailsModal.tsx#L163
Added line #L163 was not covered by tests
components/factory/forms/BurnForm.tsx
[warning] 105-105: components/factory/forms/BurnForm.tsx#L105
Added line #L105 was not covered by tests
[warning] 130-130: components/factory/forms/BurnForm.tsx#L130
Added line #L130 was not covered by tests
[warning] 146-146: components/factory/forms/BurnForm.tsx#L146
Added line #L146 was not covered by tests
[warning] 195-195: components/factory/forms/BurnForm.tsx#L195
Added line #L195 was not covered by tests
components/groups/modals/updateGroupModal.tsx
[warning] 184-184: components/groups/modals/updateGroupModal.tsx#L184
Added line #L184 was not covered by tests
components/admins/modals/validatorModal.tsx
[warning] 97-97: components/admins/modals/validatorModal.tsx#L97
Added line #L97 was not covered by tests
components/factory/forms/ConfirmationForm.tsx
[warning] 97-97: components/factory/forms/ConfirmationForm.tsx#L97
Added line #L97 was not covered by tests
[warning] 143-143: components/factory/forms/ConfirmationForm.tsx#L143
Added line #L143 was not covered by tests
components/bank/forms/ibcSendForm.tsx
[warning] 125-125: components/bank/forms/ibcSendForm.tsx#L125
Added line #L125 was not covered by tests
[warning] 129-129: components/bank/forms/ibcSendForm.tsx#L129
Added line #L129 was not covered by tests
[warning] 270-270: components/bank/forms/ibcSendForm.tsx#L270
Added line #L270 was not covered by tests
hooks/useTx.tsx
[warning] 80-85: hooks/useTx.tsx#L80-L85
Added lines #L80 - L85 were not covered by tests
components/react/inputs/AmountInput.tsx
[warning] 61-63: components/react/inputs/AmountInput.tsx#L61-L63
Added lines #L61 - L63 were not covered by tests
components/bank/forms/sendForm.tsx
[warning] 66-66: components/bank/forms/sendForm.tsx#L66
Added line #L66 was not covered by tests
[warning] 69-69: components/bank/forms/sendForm.tsx#L69
Added line #L69 was not covered by tests
[warning] 100-100: components/bank/forms/sendForm.tsx#L100
Added line #L100 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (38)
schemas/index.ts (1)
4-5: Clean addition of sendForm exportThis export makes the sendForm schema available to other components, aligning with the PR objective of moving validation schema to a separate file for better organization.
components/factory/modals/TransferModal.tsx (1)
88-88:✅ Verification successful
Good refactoring of fee estimation timing
The fee estimation is now deferred using a callback function instead of being calculated immediately. This allows the
isSigningstate to be set earlier, improving the user experience by showing loading indicators promptly.Since this is an important architectural change, let's check if it's consistent with other similar patterns:
🏁 Script executed:
#!/bin/bash # Check for the fee callback pattern across the codebase rg 'fee: \(\) =>' --type tsLength of output: 2219
Fee Estimation Callback Consistency Verified
The deferred fee estimation using a callback is implemented consistently with similar patterns across the codebase (e.g., in various admin and form modals). This approach defers the fee calculation correctly, allowing the UI to update the signing/loading state promptly.
- File Reviewed:
components/factory/modals/TransferModal.tsx(line 88)- Pattern Check: The callback form
fee: () => estimateFee(address, [msg])is consistent across multiple files, confirming the refactoring aligns with the established architectural pattern.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 88-88: components/factory/modals/TransferModal.tsx#L88
Added line #L88 was not covered by testscomponents/admins/modals/warningModal.tsx (1)
68-68: Deferred fee calculation improves UXThe change follows the same pattern implemented throughout the codebase, making fee estimation occur at transaction execution time rather than during setup.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 68-68: components/admins/modals/warningModal.tsx#L68
Added line #L68 was not covered by testscomponents/groups/forms/groups/ConfirmationForm.tsx (1)
87-87: Consistent implementation of deferred fee estimationThe change from immediate fee calculation to a callback function maintains consistency with the architectural changes across the codebase, allowing
isSigningstate to be set earlier in the transaction flow.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 87-87: components/groups/forms/groups/ConfirmationForm.tsx#L87
Added line #L87 was not covered by testscomponents/admins/modals/multiMfxMintModal.tsx (1)
103-103:❓ Verification inconclusive
Fee estimation approach improved.
The change from awaiting a pre-calculated fee to passing a function defers the fee estimation until transaction execution time. This allows the
isSigningstate to be set immediately, improving the UI responsiveness.
🏁 Script executed:
#!/bin/bash # Checking how the useTx hook handles fee functions vs pre-calculated fees ast-grep --pattern $'function useTx($_, $_) { $$$ const tx = $$$ $$$ if (typeof fee === "function") { $$$ } $$$ } $$$ }'Length of output: 151
Action Required: Manual Verification of Fee-Handling in useTx Hook
The deferred fee estimation using a function (i.e.,
fee: () => estimateFee(address, [msg])) appears to improve UI responsiveness by allowing theisSigningstate to be set immediately without waiting for fee calculation. However, the automated check for the useTx hook detecting fee functions (usingif (typeof fee === "function") { … }) did not produce any output. Please manually verify that the useTx hook properly handles fee functions as expected.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 103-103: components/admins/modals/multiMfxMintModal.tsx#L103
Added line #L103 was not covered by testscomponents/factory/forms/MintForm.tsx (1)
93-93: Fee calculation deferred to transaction execution time.Converting from a pre-calculated fee value to a function that returns the fee estimation improves the UI responsiveness by allowing the
isSigningstate to be set immediately without waiting for fee calculation to complete.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 93-93: components/factory/forms/MintForm.tsx#L93
Added line #L93 was not covered by testscomponents/admins/modals/upgradeModal.tsx (1)
155-155: Fee estimation timing improved.The change from pre-calculating the fee to passing a function that estimates the fee at execution time aligns with the PR objective of setting the
isSigningstate immediately, improving UI feedback.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 155-155: components/admins/modals/upgradeModal.tsx#L155
Added line #L155 was not covered by testscomponents/groups/modals/groupInfo.tsx (1)
143-143: Improved responsiveness for Leave Group action.By changing from pre-calculated fee to a function-based approach, the UI can now display loading indicators immediately when a user attempts to leave a group, rather than waiting for fee estimation to complete first.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 143-143: components/groups/modals/groupInfo.tsx#L143
Added line #L143 was not covered by testscomponents/groups/modals/memberManagementModal.tsx (1)
161-163: Improvement to transaction flow timingThis change correctly implements the deferred fee estimation pattern, allowing the
isSigningstate to be set immediately rather than waiting for fee estimation to complete. This enhancement will improve UX by showing loading indicators promptly when a transaction is initiated.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 162-162: components/groups/modals/memberManagementModal.tsx#L162
Added line #L162 was not covered by testscomponents/groups/modals/updateGroupModal.tsx (1)
181-190: Consistent implementation of deferred fee estimationThe fee estimation is correctly moved to be executed within the transaction boundaries, consistent with the architectural pattern being applied across the codebase. This allows for a more responsive UI by setting the
isSigningstate before performing the fee calculation.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 184-184: components/groups/modals/updateGroupModal.tsx#L184
Added line #L184 was not covered by testscomponents/factory/modals/updateDenomMetadata.tsx (1)
128-130: Fee estimation correctly deferred in token metadata updatesThe fee estimation has been properly moved to a deferred callback, consistent with the architectural changes across the codebase. This implementation works correctly for both standard transactions and group-based proposal submissions.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 129-129: components/factory/modals/updateDenomMetadata.tsx#L129
Added line #L129 was not covered by testscomponents/admins/modals/cancelUpgradeModal.tsx (1)
67-69: Deferred fee estimation with proper null handlingThe fee estimation has been correctly moved to a deferred callback, following the same pattern as other components. The defensive coding approach with
address ?? ''is properly maintained, ensuring the function won't fail if the address is null.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 68-68: components/admins/modals/cancelUpgradeModal.tsx#L68
Added line #L68 was not covered by testscomponents/admins/modals/validatorModal.tsx (1)
97-97: Changed fee calculation to deferred function executionThe fee calculation has been modified to use a callback function that will be executed at transaction time, rather than calculating it synchronously. This change aligns with the PR objective of allowing the
isSigningstate to be set immediately, improving user experience by displaying loading indicators without waiting for fee estimation to complete.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 97-97: components/admins/modals/validatorModal.tsx#L97
Added line #L97 was not covered by testscomponents/groups/modals/voteDetailsModal.tsx (3)
125-125: Changed fee calculation to deferred function executionThe fee calculation for the vote transaction has been updated to use a callback function approach, allowing the transaction signing state to be updated immediately rather than waiting for fee estimation to complete first.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 125-125: components/groups/modals/voteDetailsModal.tsx#L125
Added line #L125 was not covered by tests
142-142: Changed fee calculation to deferred function executionThe fee calculation for proposal execution has been updated to use a callback function approach, consistent with the pattern throughout this PR.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 142-142: components/groups/modals/voteDetailsModal.tsx#L142
Added line #L142 was not covered by tests
163-163: Changed fee calculation to deferred function executionThe fee calculation for proposal withdrawal has been updated to use a callback function approach, consistent with the pattern throughout this PR.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 163-163: components/groups/modals/voteDetailsModal.tsx#L163
Added line #L163 was not covered by testscomponents/admins/modals/multiMfxBurnModal.tsx (1)
100-100: Changed fee calculation to deferred function executionThe fee calculation has been modified to use a callback function that will be executed at transaction time, rather than calculating it synchronously. This matches the pattern applied across the codebase in this PR.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 100-100: components/admins/modals/multiMfxBurnModal.tsx#L100
Added line #L100 was not covered by testscomponents/factory/forms/BurnForm.tsx (4)
105-105: Removed fallback for address in proposers arrayThe nullish coalescing operator has been removed, which assumes that
addresswill always be defined at this point. This simplifies the code but requires thataddressis properly validated before reaching this code path.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 105-105: components/factory/forms/BurnForm.tsx#L105
Added line #L105 was not covered by tests
130-130: Removed fallback for address in proposers arraySimilar to the previous change, the nullish coalescing operator has been removed from the proposers array, assuming
addresswill be defined.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 130-130: components/factory/forms/BurnForm.tsx#L130
Added line #L130 was not covered by tests
146-146: Changed fee calculation to deferred function executionThe fee calculation has been modified to use a callback function that will be executed at transaction time, consistent with the pattern applied across the codebase in this PR.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 146-146: components/factory/forms/BurnForm.tsx#L146
Added line #L146 was not covered by tests
195-195: Changed fee calculation to deferred function executionThe fee calculation has been modified to use a callback function for the multi-burn transaction as well, maintaining consistency with the rest of the codebase.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 195-195: components/factory/forms/BurnForm.tsx#L195
Added line #L195 was not covered by testsschemas/sendForm.ts (3)
1-3: Good use of imports and extensions.The imports are appropriately scoped, bringing in only what's needed and using the extended Yup utility.
4-24: Well-structured validation schema with clear error messages.The validation logic for the form fields is comprehensive and includes appropriate error messaging. The schema effectively validates that:
- Recipient address is required, valid, and has the correct prefix
- Amount is required, positive, and doesn't exceed balance
- Token selection is required
46-46: Good use of type inference.Using
Yup.InferTypeto export the form type is a clean approach that ensures type definitions stay in sync with schema validation.components/factory/forms/ConfirmationForm.tsx (2)
96-98: Fee estimation is now deferred until transaction execution.The change correctly implements the deferred fee estimation pattern, which aligns with the PR objective of allowing the
isSigningstate to be set immediately.Note: This code is not covered by tests according to code coverage metrics. Consider adding tests for this scenario.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 97-97: components/factory/forms/ConfirmationForm.tsx#L97
Added line #L97 was not covered by tests
142-144: Consistent application of deferred fee estimation pattern.The same pattern is applied for the user creation transaction, ensuring consistent behavior throughout the application.
Note: This code is not covered by tests according to code coverage metrics. Consider adding tests for this scenario.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 143-143: components/factory/forms/ConfirmationForm.tsx#L143
Added line #L143 was not covered by testshooks/useTx.tsx (2)
15-17: Enhanced flexibility for fee handling.The updated type signature for the
feeproperty provides flexibility in how fees are calculated and passed to transactions. This change enables the immediate setting ofisSigningstate as mentioned in the PR objectives.
79-85: Well-implemented fee resolution logic.The new fee handling approach properly:
- Checks if a fee is provided in options
- Handles both direct fee objects and function-based fee calculations
- Falls back to estimated fees when needed
This implementation supports the PR objective of moving fee estimation within transaction execution.
Note: Consider adding tests to cover these new code paths, as they're currently not covered according to code coverage metrics.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 80-85: hooks/useTx.tsx#L80-L85
Added lines #L80 - L85 were not covered by testscomponents/bank/forms/ibcSendForm.tsx (8)
20-20: Good modularization of schema.Importing the schema from a dedicated file improves code organization and reusability.
68-70: Improved state management.Using the
isSigningstate fromuseTxhook instead of a local state variable reduces state duplication and improves consistency.
104-122: Well-extended validation schema.The validation schema appropriately extends the base
sendForm.schemawith custom validation for the recipient address based on the selected chain.
125-130: Updated type signature for better type safety.The function signature now uses the
SendFormtype from the schema, improving type safety throughout the application.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 125-125: components/bank/forms/ibcSendForm.tsx#L125
Added line #L125 was not covered by tests
[warning] 129-129: components/bank/forms/ibcSendForm.tsx#L129
Added line #L129 was not covered by tests
268-271: Consistent application of deferred fee estimation.The same pattern for deferred fee estimation is applied here, matching the implementation in other components.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 270-270: components/bank/forms/ibcSendForm.tsx#L270
Added line #L270 was not covered by tests
282-287: Type-safe initial values.Creating a strongly-typed initial value object improves code maintainability and prevents type-related errors.
574-576: Improved error handling for fee warnings.Now properly using the errors from the Formik context, which aligns with the schema-based validation approach.
609-613: Streamlined loading state handling.Using
isSigningfrom theuseTxhook for button state provides a more consistent user experience during transaction submission.components/react/inputs/AmountInput.tsx (1)
18-29: Ensure external updates are properly handled.The introduction of
internalValueandlastValuewith a useEffect correctly keeps the internal state in sync with external prop changes. However, make sure that any calls toonValueChangeoutside the component (e.g., from parent components or libraries) will not be overridden or cause unexpected re-renders if the externalvaluechanges in quick succession.components/bank/forms/sendForm.tsx (1)
100-100: Test the deferred fee estimation callback.Line 100 uses a dynamic fee callback rather than a predefined fee. Add a test confirming that
estimateFeeis called at transaction time to maintain coverage and validate correctness of drifted or variable fees.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 100-100: components/bank/forms/sendForm.tsx#L100
Added line #L100 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
components/bank/forms/ibcSendForm.tsx (2)
97-115: Consider supporting non-listed chain prefixes.
Currently, the validation defaults to'manifest'if the chain isn’t Osmosis or Axelar. This might inadvertently disallow future or unlisted chain prefixes.- .test( - 'recipient-has-prefix', - ... - function (value) { - ... - } - ) + .test('recipient-has-prefix', 'Recipient prefix must match recipient chain', function (value) { + if (!value) return true; + // If chain configs can provide a prefix, check that instead of hardcoding: + return value.startsWith(selectedToChain.prefix || 'manifest'); + })
569-572: Conditional fee warning.
Hiding the fee warning ifamounthas errors makes sense. Consider whether both messages should appear if both conditions apply.components/react/inputs/__tests__/AmountInput.test.tsx (2)
98-98: Scientific notation currently parses as invalid, reverting to 1.
If scientific notation is intended, consider expanding your regex.
135-147: Numeric precision is limited by floating-point.
Parsing0.1234567890123456789can lose some digits. If exact fractional precision is required, consider a bigger-decimal solution or instruct test runners to accept near floats.🧰 Tools
🪛 Biome (1.9.4)
[error] 146-146: This number literal will lose precision at runtime.
The value at runtime will be 0.12345678901234568
(lint/correctness/noPrecisionLoss)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
components/admins/modals/cancelUpgradeModal.tsx(1 hunks)components/admins/modals/upgradeModal.tsx(1 hunks)components/bank/forms/ibcSendForm.tsx(8 hunks)components/bank/forms/sendForm.tsx(7 hunks)components/factory/forms/BurnForm.tsx(3 hunks)components/factory/forms/MintForm.tsx(1 hunks)components/factory/modals/TransferModal.tsx(1 hunks)components/factory/modals/updateDenomMetadata.tsx(1 hunks)components/groups/forms/groups/ConfirmationForm.tsx(1 hunks)components/groups/modals/updateGroupModal.tsx(1 hunks)components/groups/modals/voteDetailsModal.tsx(3 hunks)components/react/inputs/AmountInput.tsx(3 hunks)components/react/inputs/__tests__/AmountInput.test.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- components/factory/modals/TransferModal.tsx
- components/groups/forms/groups/ConfirmationForm.tsx
- components/factory/forms/MintForm.tsx
- components/groups/modals/updateGroupModal.tsx
- components/factory/modals/updateDenomMetadata.tsx
- components/groups/modals/voteDetailsModal.tsx
- components/admins/modals/cancelUpgradeModal.tsx
- components/admins/modals/upgradeModal.tsx
- components/factory/forms/BurnForm.tsx
🧰 Additional context used
🧠 Learnings (2)
components/bank/forms/ibcSendForm.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/bank/forms/ibcSendForm.tsx:149-152
Timestamp: 2024-11-12T13:03:18.927Z
Learning: In `components/bank/forms/ibcSendForm.tsx`, the `handleSend` function relies on the `useTx` hook for error handling, so additional error handling or user notifications within `handleSend` are unnecessary.
components/bank/forms/sendForm.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/bank/forms/ibcSendForm.tsx:149-152
Timestamp: 2024-11-12T13:03:18.927Z
Learning: In `components/bank/forms/ibcSendForm.tsx`, the `handleSend` function relies on the `useTx` hook for error handling, so additional error handling or user notifications within `handleSend` are unnecessary.
🪛 Biome (1.9.4)
components/react/inputs/__tests__/AmountInput.test.tsx
[error] 146-146: This number literal will lose precision at runtime.
The value at runtime will be 0.12345678901234568
(lint/correctness/noPrecisionLoss)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (33)
components/bank/forms/ibcSendForm.tsx (10)
21-21: Looks good importingsendForm.
No issues noted.
61-61: Conversion toisSigninglooks valid.
This aligns with removing separateisSendingstates.
63-63: No concerns with theuseFeeEstimationhook usage.
Make sure the hook properly handles any potential backend errors.
118-118:handleSendsignature update.
This strongly types the form values, helping maintain consistency.
122-122: Validated approach for base unit conversion.
Ensure edge cases (large amounts, zero) are handled gracefully.
265-265: Dynamic fee estimation callback is correct.
This change defers fee estimation until the transaction is ready to be signed.
278-283: Initial values set correctly.
Providing a default of zero foramountis fine and prevents undefined errors.
291-291: Re-initialization approach.
PointingFormik’sinitialValuesatinitialValueis consistent with your new schema design.
605-605: Disable button logic seems correct.
Disabling onisSigning || !isValid || !dirtyensures the user can’t submit prematurely.
609-609: Visually consistent loading spinner.
Updating the label to a spinner whileisSigningimproves user feedback.components/react/inputs/AmountInput.tsx (7)
4-5: Adapting to numeric or undefined is a welcome improvement.
This synchronizes with your forms expecting a number rather than a string.
18-20: Storing a string representation internally is valid.
It cleanly manages partial decimal inputs.
22-29: Sync logic withlastValue.
This avoids overwriting user input when controlled props change mid-flow.
34-39: Partial decimal entry handling is correct.
Allowing.or an empty string to remain fosters a smooth user experience.
41-51: Robust parsing and fallback logic.
Reverting to the previous valid value if parsing fails is a neat solution for invalid input.
60-60: DisplayinginternalValue.
Ensures the component properly reflects user’s typed input.
62-66: Well-implemented input restrictions ononKeyDown.
Digits, backspace, arrow keys, and.remain functional while invalid chars are blocked.components/react/inputs/__tests__/AmountInput.test.tsx (8)
6-6: Removed unusedNumberInputimport.
Great cleanup.
47-47: Correctly expects numeric calls.
Switching to42.42aligns with the updated numeric callback.
62-62: Undefined for empty input.
Matches the new logic returningundefinedin partial or blank states.
77-77: Converting fractional input to an integer fallback.
Works as intended with the new numeric approach.
92-92:undefinedfor invalid input on empty initial.
Consistent with the fallback logic.
95-95: Numeric1for a valid parse.
Behavior is aligned with the updatedAmountInput.
113-114: Undefined for a lone.
Ensures partial decimal entry is recognized.
116-133: Large number check is correct.
Rejecting inputs aboveNumber.MAX_SAFE_INTEGERby falling back to the last valid value prevents precision errors.components/bank/forms/sendForm.tsx (8)
16-16: Good refactoring of schema imports.Moving the validation schema to a separate file improves modularity and maintainability, allowing for potential reuse across other components (like ibcSendForm as mentioned in the PR objectives).
58-58: Clean schema implementation.Using an imported schema instead of an inline definition improves code organization and separation of concerns.
60-60: Type safety improvement.Updating the function signature to use the strongly typed
sendForm.SendForminterface provides better type safety and self-documentation.
63-63: Handling numerical values correctly.This change correctly handles the updated AmountInput component behavior, which now returns a numerical value instead of a string as mentioned in the PR objectives.
94-94: Key improvement: Deferred fee estimation.This is the core change of the PR - moving fee estimation into a callback function that executes within the transaction bounds. This allows the isSigning state to be set immediately before fee estimation begins, providing better user feedback during transactions.
107-112: Better typed initialization.Explicitly typing initialValues as
sendForm.SendFormimproves type safety and ensures compatibility with the schema.
238-238: Improved loading state management.The consistent use of
isSigningfor controlling UI elements (MaxButton, submit button) and showing loading indicators provides better visual feedback during the transaction process, achieving the PR's main objective of enhancing the user experience.Also applies to: 272-273, 275-275
242-243: Enhanced error handling.The updated error handling for fee warnings matches the new schema structure while maintaining clear feedback to users about potential issues.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
components/bank/forms/sendForm.tsx (1)
60-94: 💡 Verification agent🧩 Analysis chain
Add test coverage for fee estimation changes.
The static analysis shows that lines 60, 63, and 94 (key changes in the PR) lack test coverage. Consider adding tests specifically for the deferred fee estimation functionality.
🏁 Script executed:
#!/bin/bash # Check if there are existing tests for the SendForm component echo "Checking for existing tests for SendForm component..." fd -e test.tsx -e spec.tsx "SendForm" | grep -v "node_modules" echo "Checking for existing test imports of SendForm..." rg -l "SendForm" --glob "*.test.tsx" --glob "*.spec.tsx"Length of output: 499
Enhance Fee Estimation Test Coverage
The static analysis indicates that the deferred fee estimation logic—specifically at lines 60, 63, and 94 in
components/bank/forms/sendForm.tsx—is not directly covered by tests. Although tests exist for the SendForm component (see files incomponents/bank/forms/__tests__/sendForm.test.tsxand note the ibcSendForm tests), they don’t appear to explicitly validate the behavior related to fee estimation.Action Items:
- Add dedicated tests that verify the fee estimation logic. In particular, ensure that:
- The conversion using
parseNumberToBigInt(with the fallback exponent) functions as expected.- The deferred
estimateFeecall (viafee: () => estimateFee(address, [msg])) is invoked with correct parameters.- Appropriate handling of edge cases or errors in fee estimation is covered.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-60: components/bank/forms/sendForm.tsx#L60
Added line #L60 was not covered by tests
[warning] 63-63: components/bank/forms/sendForm.tsx#L63
Added line #L63 was not covered by tests
[warning] 94-94: components/bank/forms/sendForm.tsx#L94
Added line #L94 was not covered by tests
🧹 Nitpick comments (8)
schemas/sendForm.ts (2)
6-9: The utility function lacks proper typing for selectedToken parameter.The
amountToBNfunction usesanyfor the selectedToken parameter type, which loses type safety. Since there's a TODO comment later about using the proper CombinedBalanceInfo type, this function should also follow that pattern.-function amountToBN(amount: string, selectedToken: any) { +function amountToBN(amount: string, selectedToken: CombinedBalanceInfo) { const exponent = selectedToken.metadata?.denom_units[1]?.exponent ?? 6; return new BigNumber(amount).div(Math.pow(10, exponent)); }
11-11: Consider making MIN_FEE_BUFFER a named constant with export.Since this value is used in validation logic, it might be useful to export it so that components can use it consistently, especially when displaying error messages or calculating available balances.
-const MIN_FEE_BUFFER = 0.09; +export const MIN_FEE_BUFFER = 0.09;components/bank/forms/ibcSendForm.tsx (2)
61-63: Improve useTx destructuring for better explicitness.The PR is refactoring to use the isSigning state from useTx instead of a local isSending state. Since the component also uses the tx function from useTx, it would be more explicit to destructure it on a single line.
- const { isSigning, tx } = useTx(env.chain); - const { setToastMessage } = useToast(); - const { estimateFee } = useFeeEstimation(env.chain); + const { isSigning, tx } = useTx(env.chain); + const { setToastMessage } = useToast(); + const { estimateFee } = useFeeEstimation(env.chain);
332-350: Remove or clarify commented-out code.There's a block of commented-out code related to a switch button that appears to be unused. It would be better to either remove it if it's no longer needed or add a comment explaining why it's being preserved.
Either remove the commented code or add a clear explanatory comment about why it's being kept:
- {/* Switch Button */} - {/* <div - onClick={e => { - e.preventDefault(); - setIsIconRotated(!isIconRotated); - setSelectedFromChain(selectedToChain); - setSelectedToChain(selectedFromChain); - }} - className={`absolute top-[calc(50%-16px)] right-0 w-full justify-center items-center cursor-pointer z-10 ${ - isGroup === true ? 'hidden' : 'flex' - }`} - > - <button className="group btn btn-xs btn-primary flex items-center justify-center hover:bg-primary-focus transition-colors"> - <TransferIcon - className={`transition-transform duration-200 cursor-pointer ${ - isIconRotated ? 'rotate-180' : '' - }`} - /> - </button> - </div> */} + {/* + TODO: Chain switching functionality temporarily disabled. + Will be re-enabled in a future PR when we implement bi-directional transfers. + */}components/react/inputs/AmountInput.tsx (1)
66-70: Added keyboard input filtering for better user experience.Good addition of the onKeyDown handler to prevent non-numeric input, improving the user experience. However, this functionality is not covered by tests according to static analysis.
Consider adding a test for the onKeyDown handler to ensure it correctly prevents non-numeric inputs:
test('prevents non-numeric keyboard input', () => { const value = '42'; const onValueChange = jest.fn(); render( <TestForm> <AmountInput name="test" value={value} onValueChange={onValueChange} /> </TestForm> ); const input = screen.getByPlaceholderText('0.00'); // Test that numeric keys are allowed const numericEvent = new KeyboardEvent('keydown', { key: '5' }); Object.defineProperty(numericEvent, 'preventDefault', { value: jest.fn() }); input.dispatchEvent(numericEvent); expect(numericEvent.preventDefault).not.toHaveBeenCalled(); // Test that non-numeric keys are prevented const alphaEvent = new KeyboardEvent('keydown', { key: 'a' }); Object.defineProperty(alphaEvent, 'preventDefault', { value: jest.fn() }); input.dispatchEvent(alphaEvent); expect(alphaEvent.preventDefault).toHaveBeenCalled(); });🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 66-68: components/react/inputs/AmountInput.tsx#L66-L68
Added lines #L66 - L68 were not covered by testscomponents/react/inputs/__tests__/AmountInput.test.tsx (1)
66-79: Clarify test description to match actual behavior.The test description "calls onValueChange with the same value when the input is invalid" is somewhat misleading. The test is actually checking that onValueChange is called with the last valid value when input is invalid, not necessarily the "same" value.
- test('calls onValueChange with the same value when the input is invalid', () => { + test('calls onValueChange with the last valid value when the input is invalid', () => {components/bank/forms/sendForm.tsx (2)
144-146: Handling numerical values properly.This change aligns with the AmountInput component now returning a numerical value instead of a string. Using
toFixed()ensures the value is properly formatted for the form.However, be cautious about potential null or undefined values. Consider adding a fallback:
onValueChange={value => { - setFieldValue('amount', value?.toFixed()); + setFieldValue('amount', value ? value.toFixed() : ''); }}
102-104: Consider removing redundant error handling.Based on previous feedback, the
useTxhook already handles errors, so this try-catch block might be redundant. However, keeping it for logging purposes is reasonable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/bank/forms/ibcSendForm.tsx(9 hunks)components/bank/forms/sendForm.tsx(8 hunks)components/react/inputs/AmountInput.tsx(2 hunks)components/react/inputs/__tests__/AmountInput.test.tsx(6 hunks)schemas/sendForm.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
components/bank/forms/sendForm.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/bank/forms/ibcSendForm.tsx:149-152
Timestamp: 2024-11-12T13:03:18.927Z
Learning: In `components/bank/forms/ibcSendForm.tsx`, the `handleSend` function relies on the `useTx` hook for error handling, so additional error handling or user notifications within `handleSend` are unnecessary.
components/bank/forms/ibcSendForm.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/bank/forms/ibcSendForm.tsx:149-152
Timestamp: 2024-11-12T13:03:18.927Z
Learning: In `components/bank/forms/ibcSendForm.tsx`, the `handleSend` function relies on the `useTx` hook for error handling, so additional error handling or user notifications within `handleSend` are unnecessary.
🪛 GitHub Check: codecov/patch
components/bank/forms/sendForm.tsx
[warning] 60-60: components/bank/forms/sendForm.tsx#L60
Added line #L60 was not covered by tests
[warning] 63-63: components/bank/forms/sendForm.tsx#L63
Added line #L63 was not covered by tests
[warning] 94-94: components/bank/forms/sendForm.tsx#L94
Added line #L94 was not covered by tests
schemas/sendForm.ts
[warning] 46-46: schemas/sendForm.ts#L46
Added line #L46 was not covered by tests
components/react/inputs/AmountInput.tsx
[warning] 66-68: components/react/inputs/AmountInput.tsx#L66-L68
Added lines #L66 - L68 were not covered by tests
components/bank/forms/ibcSendForm.tsx
[warning] 118-118: components/bank/forms/ibcSendForm.tsx#L118
Added line #L118 was not covered by tests
[warning] 122-122: components/bank/forms/ibcSendForm.tsx#L122
Added line #L122 was not covered by tests
[warning] 265-265: components/bank/forms/ibcSendForm.tsx#L265
Added line #L265 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (19)
components/bank/forms/ibcSendForm.tsx (5)
118-118: Updated handleSend function signature to leverage the new schema type.Good job updating the function signature to use the SendForm type from the schema. This ensures better type safety across the codebase when working with form values.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 118-118: components/bank/forms/ibcSendForm.tsx#L118
Added line #L118 was not covered by tests
265-265: Fee estimation is now deferred through a callback function.This aligns with the PR objective to "Move execution of fee estimation within the Tx bounds" so that isSigning state can be set immediately. The deferred callback approach allows the fee to be calculated at transaction execution time rather than beforehand.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 265-265: components/bank/forms/ibcSendForm.tsx#L265
Added line #L265 was not covered by tests
278-283: InitialValue now uses the SendForm type.Good job making the initialValue variable explicitly typed with SendForm. This ensures type consistency and helps catch potential type errors at compile time.
467-471: Updated AmountInput usage to handle BigNumber values.The component now correctly handles the updated AmountInput API, which returns BigNumber instances instead of strings. The value is properly formatted back to a string using toFixed().
573-574: Usage of error handling from schema.Good job updating the error handling to use the errors object from Formik for feeWarning, aligning with the new centralized validation logic in the schema.
components/react/inputs/AmountInput.tsx (4)
1-7: Updated component interface to return BigNumber instead of strings.The component now returns BigNumber values from onValueChange, which aligns with the PR objectives to improve interaction with schemas and Formik. This is a good change that will reduce parsing needs in parent components.
25-36: Added internal state management to handle value synchronization.Good implementation of internal state to track both the displayed value and synchronize with external value changes. The useEffect properly handles the synchronization when the value prop changes.
41-55: Improved input handling with proper type conversion.The code now correctly validates number formats and provides appropriate feedback through onValueChange. It handles edge cases like empty strings and single dots as part of number input.
58-60: Added className prop support for more flexible styling.Good improvement to allow passing custom classes to the input element. This enhances component flexibility and allows for better integration with different UI designs.
components/react/inputs/__tests__/AmountInput.test.tsx (4)
1-8: Updated imports and dependencies to match new component behavior.The imports now include BigNumber and remove NumberInput, reflecting the component's updated functionality.
47-49: Updated test assertions to match new BigNumber return values.Tests now properly expect BigNumber instances from onValueChange calls instead of strings, which aligns with the component's new API.
117-136: Added tests for very large numbers.Great addition of tests for handling large numbers beyond JavaScript's safe integer limits. This ensures the component correctly leverages BigNumber's capabilities for high-precision financial calculations.
138-150: Added test for decimal precision.Good test to verify the component maintains full decimal precision. This is important for financial applications where rounding errors can be problematic.
components/bank/forms/sendForm.tsx (6)
16-16: Good job on refactoring the validation schema!Moving the validation schema to a separate file improves maintainability and allows for reuse across components.
58-58: Clean implementation of the imported schema.Using the imported schema instead of an inline definition is a good practice for maintainability.
60-63: Type safety improvement with SendForm type.Good job using the imported type for the form values, which enhances type safety. The addition of
.toString()in line 63 ensures proper string conversion before parsing.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-60: components/bank/forms/sendForm.tsx#L60
Added line #L60 was not covered by tests
[warning] 63-63: components/bank/forms/sendForm.tsx#L63
Added line #L63 was not covered by tests
94-94: Excellent improvement to fee estimation.Moving the fee estimation inside a callback function allows the
isSigningstate to be set immediately, improving the user experience by showing loading indicators without waiting for potentially time-consuming fee estimation calls.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 94-94: components/bank/forms/sendForm.tsx#L94
Added line #L94 was not covered by tests
107-113: Clean type declaration for initial values.Using the imported type for initial values improves type safety and consistency.
241-242: Streamlined state management.Good job simplifying the component by relying on
isSigningfor controlling the disabled state and loading indicator, instead of having separateisSendingandisSigningstates.Also applies to: 275-276, 278-279
| const amountBN = new BigNumber(amount, selectedToken); | ||
| const balance = amountToBN(selectedToken.amount, selectedToken); | ||
|
|
||
| const MIN_FEE_BUFFER = 0.09; | ||
| const hasInsufficientBuffer = amount.gt(balance.minus(MIN_FEE_BUFFER)); | ||
|
|
||
| return !hasInsufficientBuffer; | ||
| }), |
There was a problem hiding this comment.
Fix fee warning calculation logic.
There are several issues in the fee warning calculation:
- Line 51 redefines
MIN_FEE_BUFFERwhich is already defined at line 11 amountis used as a BigNumber in line 52 (calling.gt()) but it's a string from the form inputamountBNis constructed incorrectly - should use theamountToBNfunction
These issues will cause the fee validation to malfunction.
- const amountBN = new BigNumber(amount, selectedToken);
+ const amountBN = amountToBN(amount, selectedToken);
const balance = amountToBN(selectedToken.amount, selectedToken);
- const MIN_FEE_BUFFER = 0.09;
- const hasInsufficientBuffer = amount.gt(balance.minus(MIN_FEE_BUFFER));
+ const hasInsufficientBuffer = amountBN.gt(balance.minus(MIN_FEE_BUFFER));
return !hasInsufficientBuffer;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const amountBN = new BigNumber(amount, selectedToken); | |
| const balance = amountToBN(selectedToken.amount, selectedToken); | |
| const MIN_FEE_BUFFER = 0.09; | |
| const hasInsufficientBuffer = amount.gt(balance.minus(MIN_FEE_BUFFER)); | |
| return !hasInsufficientBuffer; | |
| }), | |
| const amountBN = amountToBN(amount, selectedToken); | |
| const balance = amountToBN(selectedToken.amount, selectedToken); | |
| const hasInsufficientBuffer = amountBN.gt(balance.minus(MIN_FEE_BUFFER)); | |
| return !hasInsufficientBuffer; | |
| }), |
| // TODO: Use the proper type (CombinedBalanceInfo) for selectedToken. | ||
| selectedToken: Yup.mixed<any>().nullable().required('Please select a token'), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
TODO should be addressed before merging.
The schema has a TODO comment about using the proper type for selectedToken. This should be addressed to ensure type safety.
I can help implement the proper type for selectedToken. Would you like me to generate a fix for this?
🌐 Web query:
What is CombinedBalanceInfo type in TypeScript?
💡 Result:
The CombinedBalanceInfo type in TypeScript is not explicitly defined in the provided sources but can be inferred as a custom utility type designed to aggregate balance-related data from multiple sources or representations. Based on TypeScript's type-combining patterns discussed in the search results, here's how such a type might be structured:
Key Characteristics of CombinedBalanceInfo
-
Union Types
Combines multiple balance representations (e.g.,AccountBalance,CreditBalance) into a single type using|:type CombinedBalanceInfo = AccountBalance | CreditBalance;
This allows flexibility where values can represent either format[1][5].
-
Intersection Types
Merges properties from distinct interfaces (e.g., combining basic balances with transaction history):interface BaseBalance { total: number; currency: string; } interface TransactionHistory { transactions: number[]; } type CombinedBalanceInfo = BaseBalance & TransactionHistory;
Requires objects to include all properties from both types[1][5].
-
Discriminated Unions
Uses a shared literal field (e.g.,type) to differentiate between balance scenarios:type CombinedBalanceInfo = | { type: "checking"; balance: number; overdraftLimit: number; } | { type: "savings"; balance: number; interestRate: number; };
Enables type-safe conditional logic via checks on
type[5][8]. -
Mapped Types for Clarity
Flattens intersections to improve IDE hints:type Spread<T> = { [K in keyof T]: T[K] }; type CombinedBalanceInfo = Spread<AccountBalance & CreditBalance>;
Reveals all combined properties explicitly[3].
Example Implementation
// Base types
interface CryptoBalance {
btc: number;
eth: number;
}
interface FiatBalance {
usd: number;
eur: number;
}
// Combined type using intersection + mapped type
type CombinedBalanceInfo = Spread<CryptoBalance & FiatBalance>;
// Resulting structure:
// {
// btc: number;
// eth: number;
// usd: number;
// eur: number;
// }Use Cases
- Multi-currency wallets: Aggregating crypto and fiat balances.
- Financial dashboards: Merging checking/savings account data.
- API responses: Standardizing heterogeneous balance formats[8][3].
This pattern ensures type safety while accommodating diverse data structures common in financial applications[5][8].
Citations:
- 1: https://ncoughlin.com/posts/typescript-combining-types
- 2: https://www.wwt.com/article/typescript-records-and-mapped-types
- 3: https://dev.to/kirkcodes/revealing-compound-types-in-typescript-2ic8
- 4: https://www.typescriptlang.org/docs/handbook/advanced-types.html
- 5: https://www.typescriptlang.org/docs/handbook/unions-and-intersections.html
- 6: https://www.youtube.com/watch?v=Kk0VlZtHUUI
- 7: https://www.typescriptlang.org/docs/handbook/type-compatibility.html
- 8: https://www.rahulpnath.com/blog/typescript-sum-types/
- 9: https://dev.to/room_js/typescript-how-do-you-share-type-definitions-across-multiple-projects-1203
- 10: https://www.dhiwise.com/post/writing-clearer-code-with-typescript-pick-comprehensive-guide
ACTION REQUIRED: Replace the generic type with CombinedBalanceInfo to enforce proper type safety
In file schemas/sendForm.ts (lines 37-38), the selectedToken field is currently defined as:
// TODO: Use the proper type (CombinedBalanceInfo) for selectedToken.
selectedToken: Yup.mixed<any>().nullable().required('Please select a token'),Based on the intended purpose of CombinedBalanceInfo—a custom utility type to aggregate balance-related data—you should:
- Define CombinedBalanceInfo: Ensure that a proper type is defined (e.g., using union, intersection, or discriminated union patterns) to capture the expected balance data shape.
- Update the schema: Replace
Yup.mixed<any>()with a schema that leverages theCombinedBalanceInfotype to improve type safety.
Please address this before merging to maintain consistency and robustness in our type definitions.
This allows for isSigning to be set right away, before proceeding
to estimating fees, which can take a while. Thus every component
reacting to isSigning (and showing the three bouncing dots) will
be disabled right away, without waiting for server requests to
finish.
Also moved the validation schema for sendForm to its own schema
file and import. I did not want to touch ibcSendForm, but the
idea would be to reuse the schema there too. Will do in a follow
up PR.
Finally, fixed some internals with AmountInput and the way it
interacts with schemas and Formik. This new version returns only
actual number (or undefined) instead of a string that needs
parsing. This makes it easier to use in most cases too.
Fixes #308
Summary by CodeRabbit
Refactor
New Features
Tests