-
Notifications
You must be signed in to change notification settings - Fork 51
currencyCode -> tokenId followup fixes #1008
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| { | ||
| name: 'USD.st', | ||
| name: 'USD', | ||
| multiplier: '1000000000000000000' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Ripple migration loses data due to renamed currency codes
The Ripple token currency codes were changed from unique values (USD.gh, USD.bs, USD.st, EUR.gh, EUR.bs) to duplicated simple values (USD, EUR). The migrateCurrencyCodeToTokenId function looks up tokens by matching currencyCode against stored keys from old data. Since the old currency codes like USD.gh no longer exist in any token definition, the migration will skip these entries (continue at line 359), causing transaction history loss for users with existing Ripple token data stored under the old currency code keys.
Additional Locations (1)
| txIdMap[''] == null) || | ||
| (transactionList != null && | ||
| Object.keys(transactionList).length > 0 && | ||
| transactionList[''] == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Re-migration destroys already-migrated token-only wallet data
The needsMigration heuristic uses data[''] == null to detect unmigrated data, assuming migrated data always has an empty string key for native currency. However, wallets with only token transactions (no native currency transactions) won't have this key after migration. On subsequent loads, the heuristic incorrectly triggers re-migration on already-migrated data. When migrateCurrencyCodeToTokenId processes tokenId keys (like contract addresses), they don't match any token's currencyCode, causing all entries to be silently dropped via the continue statement, resulting in complete transaction history loss.
Additional Locations (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor makes a fair point. Would it be possible to set txIdList[''] ?= [] and so forth to ensure we never double-migrate? Otherwise, perhaps adding a otherData.tokenIdMigrated flag could prevent double migrations.
|
swansontec
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good overall, just one safety thing would be good to add.
| txIdMap[''] == null) || | ||
| (transactionList != null && | ||
| Object.keys(transactionList).length > 0 && | ||
| transactionList[''] == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor makes a fair point. Would it be possible to set txIdList[''] ?= [] and so forth to ensure we never double-migrate? Otherwise, perhaps adding a otherData.tokenIdMigrated flag could prevent double migrations.
a006947 to
6f43ec2
Compare
6f43ec2 to
a006a37
Compare
The transactions have to be queried newest to oldest so the function is written to retry regardless of errors.
This preserves the balance handling that existed prior to the tokenId upgrade
a006a37 to
6b5b582
Compare
| const currentBalance = this.getBalance({ tokenId }) | ||
| if (currentBalance == null || !eq(balance, currentBalance)) { | ||
| this.updateBalance(tokenId, balance) | ||
| this.walletLocalData.totalBalances[''] = balance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: ZcashEngine ignores tokenId when storing balance
The updateBalance method stores the balance using a hardcoded '' key instead of using the tokenId parameter as tokenId ?? '' like the base class does. This means the tokenId parameter is ignored for storage, and any balance update would incorrectly write to the native token's key regardless of which token was specified. The callback on line 224 correctly uses tokenId, creating an inconsistency where the reported token differs from where the balance is actually stored.
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
noneNote
Migrates on‑disk transaction data to
tokenIdkeys, refactors Ethereum update flow for partial status, normalizes Ripple token codes, adds Thorchain rate‑limit backoff, and fixes Zcash balance update recursion.currencyCodekeys totokenIdkeys with in-engine migration inCurrencyEngine.loadTransactions()and helpermigrateCurrencyCodeToTokenId().tokenIdkey''and includenullin sync progress (updateOnAddressesChecked).EthereumNetwork.acquireUpdates()to build update tasks (balances/txs), run them with error aggregation, and allow partial status updates.tokenId: null) as well inprocessEthereumNetworkUpdate.asEthereumWalletOtherData; simplifyEthereumEngine.setOtherData.USD/EURinsrc/ripple/rippleInfo.ts.snoozebefore throwing on non-OK Midgard responses to mitigate rate limits inThorchainEngine.queryTransactions().ZcashEngine.updateBalanceto avoid recursive call; directly updatestotalBalances['']and callback.tokenIdkeys and ETH update refactor.Written by Cursor Bugbot for commit 6b5b582. This will update automatically on new commits. Configure here.