fix: sip10 token send form fees bug #5414
Conversation
|
Warning Rate Limit Exceeded@alter-eggo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 55 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe changes encompass optimizing navigation for sending SIP10 tokens, refining fee estimation, strengthening error handling in transaction generation, introducing a standardized address extraction function, and ensuring uniformity in address retrieval methods across functions. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| export const getAssetContractAddress = (value: string): string => { | ||
| if (value.includes('.')) { | ||
| return value.split('.')[0]; | ||
| } | ||
|
|
||
| return value; | ||
| }; |
There was a problem hiding this comment.
Can use fn declaration as we do elsewhere
There was a problem hiding this comment.
Also should have a unit test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/app/pages/send/send-crypto-asset-form/form/sip10/sip10-token-send-form.tsx (2 hunks)
- src/app/ui/utils/get-asset-contract-address.ts (1 hunks)
- src/app/ui/utils/get-asset-string-parts.ts (2 hunks)
Additional comments not posted (2)
src/app/ui/utils/get-asset-contract-address.ts (1)
8-14: The implementation ofgetAssetContractAddressappears correct and efficient for extracting the contract address from a string.src/app/pages/send/send-crypto-asset-form/form/sip10/sip10-token-send-form.tsx (1)
25-32: Switching touseNavigatefor programmatic navigation aligns with modern React Router practices and is implemented correctly here.
| @@ -1,3 +1,4 @@ | |||
| import { getAssetContractAddress } from './get-asset-contract-address'; | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The fallback logic in getAssetStringParts assigns the same string to both contractName and assetName when the input string does not contain expected delimiters. This might not be the intended behavior.
src/app/ui/utils/get-asset-string-parts.ts: Lines 8-16
Please review and ensure that the fallback logic correctly assigns contractName and assetName.
Analysis chain
Refactoring to use getAssetContractAddress enhances consistency in address extraction. However, ensure that the fallback logic (when delimiters are missing) correctly assigns contractName and assetName.
Also applies to: 27-33
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the fallback logic for `getAssetStringParts` when delimiters are missing.
# Test: Search for usages of `getAssetStringParts` and review the expected input patterns.
rg --type typescript $'getAssetStringParts'
Length of output: 77
Script:
#!/bin/bash
# Description: Verify the fallback logic for `getAssetStringParts` when delimiters are missing.
# Test: Search for usages of `getAssetStringParts` and review the expected input patterns.
rg --type ts 'getAssetStringParts'
Length of output: 813
Script:
#!/bin/bash
# Extract the relevant lines from `token-transfer.hooks.ts` and `sip10-tokens.utils.ts` to review the usage of `getAssetStringParts`.
# Extract lines around the usage in `token-transfer.hooks.ts`
rg --type ts -A 5 -B 5 'getAssetStringParts' src/app/store/transactions/token-transfer.hooks.ts
# Extract lines around the usage in `sip10-tokens.utils.ts`
rg --type ts -A 5 -B 5 'getAssetStringParts' src/app/query/stacks/sip10/sip10-tokens.utils.ts
Length of output: 1927
Script:
#!/bin/bash
# Extract the implementation of `getAssetStringParts` to review the fallback logic.
ast-grep --lang typescript --pattern $'export const getAssetStringParts = ($_): $_ {
$$$
}' src/app/ui/utils/get-asset-string-parts.ts
Length of output: 137
Script:
#!/bin/bash
# Extract the implementation of `getAssetStringParts` to review the fallback logic.
ast-grep --lang typescript --pattern 'export const getAssetStringParts = ($$) => {
$$$
}' src/app/ui/utils/get-asset-string-parts.ts
Length of output: 136
Script:
#!/bin/bash
# Extract the implementation of `getAssetStringParts` to review the fallback logic.
rg 'export const getAssetStringParts' -A 20 src/app/ui/utils/get-asset-string-parts.ts
Length of output: 683
alter-eggo
force-pushed
the
fix/sip10-send-form
branch
2 times, most recently
from
a6246b5 to
a341107
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/app/pages/send/send-crypto-asset-form/form/sip10/sip10-token-send-form.tsx (2 hunks)
- src/app/query/stacks/fees/fees.query.ts (1 hunks)
- src/app/store/transactions/token-transfer.hooks.ts (2 hunks)
- src/app/ui/utils/get-asset-contract-address.ts (1 hunks)
- src/app/ui/utils/get-asset-string-parts.ts (2 hunks)
- src/app/ui/utils/stacks-ft-utils.spec.ts (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/app/pages/send/send-crypto-asset-form/form/sip10/sip10-token-send-form.tsx
- src/app/ui/utils/get-asset-contract-address.ts
- src/app/ui/utils/get-asset-string-parts.ts
Additional Context Used
GitHub Check Runs (1)
test-unit failure (4)
src/app/ui/utils/stacks-ft-utils.spec.ts: [failure] 12-12: src/app/ui/utils/stacks-ft-utils.spec.ts > getAssetContractAddress > should return the contract address from a fully qualified asset name
AssertionError: expected 'SP2KAF9RF86PVX3NEE27DFV1CQX0T4WGR41X3…' to be 'SP2J6ZY48GV1EZ5V2V5RB9MP66SW86PYKKQW2…' // Object.is equality
- Expected
- Received
- SP2J6ZY48GV1EZ5V2V5RB9MP66SW86PYKKQW2S7D0
- SP2KAF9RF86PVX3NEE27DFV1CQX0T4WGR41X3S45C
❯ src/app/ui/utils/stacks-ft-utils.spec.ts:12:34
src/app/ui/utils/stacks-ft-utils.spec.ts: [failure] 17-17: src/app/ui/utils/stacks-ft-utils.spec.ts > getAssetContractAddress > should return the contract address from a contract id
AssertionError: expected 'SP2KAF9RF86PVX3NEE27DFV1CQX0T4WGR41X3…' to be 'SP2J6ZY48GV1EZ5V2V5RB9MP66SW86PYKKQW2…' // Object.is equality
- Expected
- Received
- SP2J6ZY48GV1EZ5V2V5RB9MP66SW86PYKKQW2S7D0
- SP2KAF9RF86PVX3NEE27DFV1CQX0T4WGR41X3S45C
❯ src/app/ui/utils/stacks-ft-utils.spec.ts:17:41
Additional comments not posted (3)
src/app/ui/utils/stacks-ft-utils.spec.ts (1)
22-22: This test case correctly verifies the contract address when called with a contract address.src/app/query/stacks/fees/fees.query.ts (1)
26-26: The addition of thepriority: 2option in the axios request configuration is a good enhancement for handling transaction fee estimations.src/app/store/transactions/token-transfer.hooks.ts (1)
90-153: The addition of error handling and logging inuseGenerateFtTokenTransferUnsignedTxenhances the robustness of the transaction generation process.
|
With this latest build I can select a token. It doesn't seem to load fees but defaults to fixed low fee for low, medium and high. When I add an amount that is not available I get no warning about insufficient funds. Then I can continue to review |
alter-eggo
force-pushed
the
fix/sip10-send-form
branch
from
a341107 to
40371b5
Compare
| address: contractAddress, | ||
| contractName, | ||
| assetName, | ||
| } = getAssetStringParts(info.contractId); |
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!pnpm-lock.yaml
Files selected for processing (5)
- package.json (1 hunks)
- src/app/pages/send/send-crypto-asset-form/form/sip10/sip10-token-send-form.tsx (2 hunks)
- src/app/query/stacks/fees/fees.query.ts (1 hunks)
- src/app/query/stacks/sip10/sip10-tokens.utils.ts (1 hunks)
- src/app/store/transactions/token-transfer.hooks.ts (3 hunks)
Files skipped from review due to trivial changes (2)
- package.json
- src/app/query/stacks/fees/fees.query.ts
Files skipped from review as they are similar to previous changes (1)
- src/app/pages/send/send-crypto-asset-form/form/sip10/sip10-token-send-form.tsx
Additional comments not posted (2)
src/app/query/stacks/sip10/sip10-tokens.utils.ts (2)
24-24: Proper use of nullish coalescing and logical reuse ofisTransferableSip10Token. Well done on maintaining clean and robust code.
19-24: Refactored to usegetAssetStringPartsfor parsing asset details. Ensure integration with other parts of the application.Verification successful
Integration of
getAssetStringPartsverified. The function is defined insrc/app/ui/utils/get-asset-string-parts.tsand used consistently insrc/app/query/stacks/sip10/sip10-tokens.utils.ts.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of `getAssetStringParts` across the application. # Test: Search for usage of `getAssetStringParts`. Expect: Consistent usage across the application. rg --type typescript $'getAssetStringParts'Length of output: 77
Script:
#!/bin/bash # Description: Verify integration of `getAssetStringParts` across the application. # Test: Search for usage of `getAssetStringParts`. Expect: Consistent usage across the application. rg --type ts 'getAssetStringParts'Length of output: 596
| stxAddress: account.address, | ||
| }; | ||
|
|
||
| const postConditions = [makePostCondition(postConditionOptions)]; | ||
|
|
||
| // (transfer (uint principal principal) (response bool uint)) | ||
| const functionArgs: ClarityValue[] = [ | ||
| uintCV(amountAsFractionalUnit), | ||
| standardPrincipalCVFromAddress(createAddress(account.address)), | ||
| standardPrincipalCVFromAddress(recipient), | ||
| ]; | ||
|
|
||
| if (info.hasMemo) { | ||
| functionArgs.push(memo); | ||
| } | ||
|
|
||
| const options = { | ||
| txData: { | ||
| txType: TransactionTypes.ContractCall, | ||
| try { | ||
| if (!account) return; | ||
|
|
||
| const functionName = 'transfer'; | ||
| const recipient = | ||
| values && 'recipient' in values | ||
| ? createAddress(values.recipient || '') | ||
| : createEmptyAddress(); | ||
| const amount = values && 'amount' in values ? values.amount : 0; | ||
| const memo = | ||
| values && 'memo' in values && values.memo !== '' | ||
| ? someCV(bufferCVFromString(values.memo || '')) | ||
| : noneCV(); | ||
|
|
||
| const amountAsFractionalUnit = ftUnshiftDecimals(amount, info.decimals || 0); | ||
| const postConditionOptions = { | ||
| amount: amountAsFractionalUnit, | ||
| contractAddress, | ||
| contractAssetName, | ||
| contractName, | ||
| functionName, | ||
| functionArgs: functionArgs.map(serializeCV).map(arg => bytesToHex(arg)), | ||
| postConditions, | ||
| postConditionMode: PostConditionMode.Deny, | ||
| network, | ||
| stxAddress: account.address, | ||
| }; | ||
|
|
||
| const postConditions = [makePostCondition(postConditionOptions)]; | ||
|
|
||
| // (transfer (uint principal principal) (response bool uint)) | ||
| const functionArgs: ClarityValue[] = [ | ||
| uintCV(amountAsFractionalUnit), | ||
| standardPrincipalCVFromAddress(createAddress(account.address)), | ||
| standardPrincipalCVFromAddress(recipient), | ||
| ]; | ||
|
|
||
| if (info.hasMemo) { | ||
| functionArgs.push(memo); | ||
| } | ||
|
|
||
| const options = { | ||
| txData: { | ||
| txType: TransactionTypes.ContractCall, | ||
| contractAddress, | ||
| contractName, | ||
| functionName, | ||
| functionArgs: functionArgs.map(serializeCV).map(arg => bytesToHex(arg)), | ||
| postConditions, | ||
| postConditionMode: PostConditionMode.Deny, | ||
| network, | ||
| publicKey: account.stxPublicKey, | ||
| }, | ||
| fee: stxToMicroStx(values?.fee || 0).toNumber(), | ||
| publicKey: account.stxPublicKey, | ||
| }, | ||
| fee: stxToMicroStx(values?.fee || 0).toNumber(), | ||
| publicKey: account.stxPublicKey, | ||
| nonce: Number(values?.nonce) ?? nextNonce?.nonce, | ||
| } as const; | ||
| nonce: Number(values?.nonce) ?? nextNonce?.nonce, | ||
| } as const; | ||
|
|
||
| return generateUnsignedTransaction(options); | ||
| return generateUnsignedTransaction(options); | ||
| } catch (error) { | ||
| logger.error('Failed to generate unsigned transaction', error); | ||
| return; | ||
| } | ||
| }, | ||
| [account, info.contractId, info.decimals, info.hasMemo, network, nextNonce?.nonce] | ||
| [ | ||
| account, | ||
| info.decimals, | ||
| info.hasMemo, | ||
| network, | ||
| nextNonce?.nonce, | ||
| contractName, | ||
| contractAssetName, | ||
| contractAddress, | ||
| ] |
There was a problem hiding this comment.
Comprehensive refactor of useGenerateFtTokenTransferUnsignedTx to include detailed error handling and dynamic construction of transaction details. Consider adding unit tests to cover these new functionalities.
Would you like assistance in setting up unit tests for this function?
|
Let's ship? 🚢 |
|
Retested this version fix/sip10-send-form#40371b5, doesn't work.
|
|
fee setting by the wallet does look correct now. |
alter-eggo
force-pushed
the
fix/sip10-send-form
branch
from
40371b5 to
4900e5d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!pnpm-lock.yaml
Files selected for processing (10)
- package.json (1 hunks)
- src/app/common/validation/forms/amount-validators.ts (3 hunks)
- src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-item.tsx (1 hunks)
- src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-list-unsupported.tsx (1 hunks)
- src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-list.tsx (1 hunks)
- src/app/pages/send/send-crypto-asset-form/form/sip10/sip10-token-send-form.tsx (2 hunks)
- src/app/pages/send/send-crypto-asset-form/form/sip10/use-sip10-send-form.tsx (2 hunks)
- src/app/query/stacks/fees/fees.query.ts (1 hunks)
- src/app/query/stacks/sip10/sip10-tokens.utils.ts (1 hunks)
- src/app/store/transactions/token-transfer.hooks.ts (3 hunks)
Files skipped from review due to trivial changes (3)
- src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-list-unsupported.tsx
- src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-list.tsx
- src/app/pages/send/send-crypto-asset-form/form/sip10/use-sip10-send-form.tsx
Files skipped from review as they are similar to previous changes (5)
- package.json
- src/app/pages/send/send-crypto-asset-form/form/sip10/sip10-token-send-form.tsx
- src/app/query/stacks/fees/fees.query.ts
- src/app/query/stacks/sip10/sip10-tokens.utils.ts
- src/app/store/transactions/token-transfer.hooks.ts
Additional comments not posted (3)
src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-item.tsx (1)
23-23: LGTM! The usage ofspamFilteroninfo.tokenNameis a good practice to ensure clean data display.src/app/common/validation/forms/amount-validators.ts (2)
15-15: LGTM! The import offtDecimalsis correctly placed and used in the context of fungible token amount validation.
Line range hint
113-130: LGTM! ThestacksFungibleTokenAmountValidatorcorrectly handles decimal precision and balance checks usingftDecimals.
alter-eggo
force-pushed
the
fix/sip10-send-form
branch
from
4900e5d to
2ed7544
Compare
There was a problem hiding this comment.
@alter-eggo LGTM!
tested sending with several SIP010 tokens no issues with the latest build: fix/sip10-send-form#2ed7544
This pr fixes sip10 send form fees bug, discussed:
https://trustmachines.slack.com/archives/C0520G8J33M/p1716339282877119
https://trustmachines.slack.com/archives/C05LAP952E7/p1716363736626209
Summary by CodeRabbit
New Features
Improvements
Refactor
Navigatecomponent withuseNavigatehook for more efficient navigation handling.