Use tokenId instead of currencyCode #5855
Conversation
We can leave currency code filtering for scenes like the coin ranking details
We do add getWalletTokenId use in the SwapDetailsCard and that's ok because it seems that only the cosmosibc swap provider even provides uses it
Only EdgeProvider remains, which relies on selectedCurrencyCode
They are low volume or dead
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| plugin, | ||
| refundAddress | ||
| } = swapData | ||
| } = upgradeSwapData(wallet, swapData) |
There was a problem hiding this comment.
Bug: Wrong wallet passed to upgradeSwapData function
The upgradeSwapData function is called with wallet (the source wallet) instead of destinationWallet (the payout wallet). This causes the function to look up payoutTokenId or payoutCurrencyCode in the wrong wallet's currency configuration, potentially returning incorrect token information for the swap destination asset.
| "detect-bundler": "^1.1.0", | ||
| "disklet": "^0.5.2", | ||
| "edge-core-js": "^2.35.0", | ||
| "edge-core-js": "file:../edge-core-js", |
There was a problem hiding this comment.
Bug: Local file dependency in package.json
The edge-core-js dependency is changed to "file:../edge-core-js", which is a local file reference for development. This breaks the build for anyone without that specific directory structure and should not be committed to the repository. The dependency should reference a published version or a git URL instead.
swansontec
left a comment
swansontec
left a comment
There was a problem hiding this comment.
I notice that this PR doesn't update ESLint exceptions, so edited files might have leftover warnings. This is fine for a change of this magnitude, but it might cause problems in the merge-to-staging and merge-to-master, where we need to turn of the precommit thing as part of the merge. Just something to be aware of.
| const currencyCode = getCurrencyCode( | ||
| { | ||
| currencyConfig, | ||
| currencyInfo: currencyConfig.currencyInfo | ||
| } as unknown as EdgeCurrencyWallet, | ||
| tokenId | ||
| ) |
There was a problem hiding this comment.
This is fine. We need to fix getCurrencyCode to take EdgeCurrencyConfig instead of EdgeCurrencyWallet.
The need to fix getCurrencyCode becomes more urgent, but I think this is a good motivation / reminder to do this soon.
| const contractAddress = `0x${tokenId}`.toLocaleUpperCase() | ||
| return `${asset}-${contractAddress}` |
There was a problem hiding this comment.
This does not work in general, since different chains use different contract address mappings. We should add a comment that this is EVM-only (which is fine because Coreum is EVM).
There was a problem hiding this comment.
My understanding is that the only tokens supported on tcsavers were evm tokens, but regardless it's a dead staking plugin.
| const tokenId = | ||
| contractAddress == null | ||
| ? null | ||
| : contractAddress.toLowerCase().replace('0x', '') |
There was a problem hiding this comment.
Same deal here - this conversion is EVM only, so we need a comment.
There was a problem hiding this comment.
Same as the other tcsavers plugin. It's dead and should be pared down to tcy functionality only.
|
|
||
| return `${asset}-${contractAddress.toLocaleUpperCase()}` | ||
| if (tokenId != null) { | ||
| const contractAddress = `0x${tokenId}`.toLocaleUpperCase() |
There was a problem hiding this comment.
Also needs a comment about being EVM only.
Oh selectedTokenId, many may not know your name
| const { multiplier } = getExchangeDenom( | ||
| coreWallet.currencyConfig, | ||
| displayCurrencyCode | ||
| ) |
There was a problem hiding this comment.
Bug: Passing currencyCode instead of tokenId to getExchangeDenom
The getExchangeDenom function expects an EdgeTokenId (contract address string or null) but is being passed displayCurrencyCode (a currency code like "BTC" or "USDT"). This type mismatch causes the function to fail to find the token in allTokens and return emptyEdgeDenomination with multiplier "1", resulting in incorrect native amount calculations for token transactions.
Additional Locations (2)
There was a problem hiding this comment.
Yes, we need to pass the tokenId here.
| const tokenId = selectedWallet?.tokenId ?? null | ||
| const { id: walletId, tokenId } = useSelector( | ||
| state => state.ui.settings.mostRecentWallets[0] | ||
| ) |
There was a problem hiding this comment.
Bug: Potential crash accessing empty mostRecentWallets array
The code destructures state.ui.settings.mostRecentWallets[0] directly without checking if the array is empty. When a new user has no recent wallets, accessing index [0] returns undefined, and destructuring { id: walletId, tokenId } from undefined will throw a runtime error. This scene will crash for users with no recent wallet history.
There was a problem hiding this comment.
If the most recent wallets array is empty, the selector will definitely crash. Adding a simple ?? { walletId: '', tokenId: null } to the selector would solve this.
| } | ||
| onAmountsChanged={() => {}} | ||
| onMaxSet={handleMaxButtonPress(currencyCode)} | ||
| onMaxSet={handleMaxButtonPress(tokenId, currencyCode)} |
There was a problem hiding this comment.
Bug: Missing tokenId when updating changeQuoteRequest in StakeModifyScene
When updating changeQuoteRequest after the user enters an amount via FlipInputModal, only currencyCode and nativeAmount are set, but tokenId is not included despite being available in the closure scope. Similarly, the useEffect that initializes the request for claim/liquidStaking/mustMaxUnstake cases sets currencyCode and nativeAmount but omits tokenId from stakePolicy.rewardAssets[0].tokenId. This causes changeQuoteRequest.tokenId to remain at its initial value of null, leading to incorrect behavior when staking tokens rather than native assets.
Additional Locations (1)
swansontec
left a comment
swansontec
left a comment
There was a problem hiding this comment.
One more round of fixes needed, but we are getting close!
| const { multiplier } = getExchangeDenom( | ||
| coreWallet.currencyConfig, | ||
| displayCurrencyCode | ||
| ) |
There was a problem hiding this comment.
Yes, we need to pass the tokenId here.
|
|
||
| const { multiplier } = getExchangeDenom( | ||
| coreWallet.currencyConfig, | ||
| displayCurrencyCode |
There was a problem hiding this comment.
We need to pass the tokenId here too.
| ) | ||
| const { multiplier } = getExchangeDenom( | ||
| coreWallet.currencyConfig, | ||
| displayCurrencyCode |
There was a problem hiding this comment.
This should be a tokenId.
| export const asMostRecentWallet = asObject({ | ||
| id: asString, | ||
| currencyCode: asString | ||
| tokenId: asEdgeTokenId |
There was a problem hiding this comment.
We can't really change this without blowing away user settings - no good! We are kinda stuck with the way the data is.
Idea: Keep the data as-is, lookup the wallet by its walletId, and then search through enabled tokens to find a matching currency code. Then store the walletId + tokenId in memory. This keeps the data the same on disk, and even if the app has multiple tokens of the same name, it won't be a problem unless the user has them both enabled.
Once we have this migration code, we can optionally write the data back to disk as walletId + tokenId, and the migration will be complete. Or just leave it as-is and rely on the enabledTokenId search - it's fine.
There was a problem hiding this comment.
We'd only be losing the mostRecentWallets as it's guarded by asMaybe. I think that is ok?
| const tokenId = selectedWallet?.tokenId ?? null | ||
| const { id: walletId, tokenId } = useSelector( | ||
| state => state.ui.settings.mostRecentWallets[0] | ||
| ) |
There was a problem hiding this comment.
If the most recent wallets array is empty, the selector will definitely crash. Adding a simple ?? { walletId: '', tokenId: null } to the selector would solve this.
| >() | ||
| const paymentCurrencyCode = | ||
| activationPaymentInfo?.tokenId !== undefined | ||
| ? getCurrencyCode(existingWallet, activationPaymentInfo?.tokenId) |
There was a problem hiding this comment.
Bug: Wrong wallet used for payment currency lookup
The getCurrencyCode and getExchangeDenom calls use existingWallet (the wallet being activated) instead of the payment wallet to look up the payment currency. The tokenId in activationPaymentInfo refers to the payment currency (e.g., BTC), but looking it up on the existing wallet (e.g., EOS) would return the wrong currency code and denomination. For example, if activating an EOS wallet by paying with BTC, this would display "EOS" instead of "BTC" for the payment currency.
Additional Locations (1)
peachbits
force-pushed
the
matthew/sans-currency-code
branch
from
d8be217 to
f442403
Compare
| } | ||
|
|
||
| return swapData | ||
| } |
There was a problem hiding this comment.
Bug: upgradeSwapData mutates props causing React anti-pattern
The upgradeSwapData function directly mutates the swapData object passed as a prop (swapData.payoutTokenId = ...). This is a React anti-pattern that can cause unexpected behavior, as props should be treated as immutable. The function should create a new object with the updated property instead of modifying the input object directly.
| export const asMostRecentWallet = asObject({ | ||
| id: asString, | ||
| currencyCode: asString | ||
| tokenId: asEdgeTokenId |
There was a problem hiding this comment.
Bug: mostRecentWallets data migration causes user data loss
The asMostRecentWallet cleaner was changed from expecting { id, currencyCode } to { id, tokenId }. Existing user data stored with currencyCode will fail to parse when loaded, causing asMaybe to fall back to an empty array. This silently erases the user's most recent wallets list when they upgrade. The PR discussion shows the reviewer suggested a migration approach to preserve the data, but this cleaner change does not include migration logic.
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
EdgeApp/edge-core-js#688
Requirements
If you have made any visual changes to the GUI. Make sure you have:
Note
Replaces currencyCode-based logic with tokenId across state, selectors, actions, UI, staking/loans/ramps/providers, and bumps edge-core-js; also restructures FIO state and recent wallet tracking.
currencyCodewithtokenIdin actions, reducers, navigation params, and settings init; removeUI/WALLETSselected wallet/currency reducers; movefioWalletstoui.fioand addUPDATE_FIO_WALLETS.{ id, tokenId }.getCurrencyCodeMultiplierandselectDisplayDenomByCurrencyCode; usegetExchangeDenomeverywhere.CryptoAmount, currency helpers, and fiat/crypto conversions to usetokenId.tokenId(AddressTile2, Tx rows, SwapDetailsCard upgrade, SendScene2, EdgeProviderComponent, WalletList, DevTestScene, staking/coins scenes).tokenIdthrough staking policies/adapters; enable tokens viaenableTokens; addLOAN_TOKEN_IDSand use in loan scenes.getExchangeDenomand passtokenIdtoCryptoAmountand spend flows.tokenIdusage and FIO state path.edge-core-jsto 2.36.0 (local); update Podfile.lock.Written by Cursor Bugbot for commit f442403. This will update automatically on new commits. Configure here.