-
Notifications
You must be signed in to change notification settings - Fork 51
Use confirmations api #1009
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
Use confirmations api #1009
Conversation
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: Migration fails for renamed Ripple token currency codes
Multiple Ripple tokens had their currency codes changed from unique identifiers (e.g., USD.gh, USD.bs, USD.st, EUR.gh, EUR.bs) to just USD and EUR. The migrateCurrencyCodeToTokenId function looks up tokens by matching currencyCode, but old on-disk data uses the previous currency codes as keys. Since no token now has currencyCode === 'USD.gh', the lookup returns undefined and the old transaction data is silently discarded. This causes data loss for users with transaction history in these tokens.
src/ripple/rippleInfo.ts#L79-L148
edge-currency-accountbased/src/ripple/rippleInfo.ts
Lines 79 to 148 in 4a2623c
| 'USD-rhub8VRN55s94qWKDv6jmDy1pUykJzF3wq': { | |
| currencyCode: 'USD', | |
| displayName: 'Gatehub USD', | |
| denominations: [ | |
| { | |
| name: 'USD', | |
| multiplier: '1000000000000000000' | |
| } | |
| ], | |
| networkLocation: { | |
| currency: 'USD', | |
| issuer: 'rhub8VRN55s94qWKDv6jmDy1pUykJzF3wq' | |
| } | |
| }, | |
| 'EUR-rhub8VRN55s94qWKDv6jmDy1pUykJzF3wq': { | |
| currencyCode: 'EUR', | |
| displayName: 'Gatehub EUR', | |
| denominations: [ | |
| { | |
| name: 'EUR', | |
| multiplier: '1000000000000000000' | |
| } | |
| ], | |
| networkLocation: { | |
| currency: 'EUR', | |
| issuer: 'rhub8VRN55s94qWKDv6jmDy1pUykJzF3wq' | |
| } | |
| }, | |
| 'USD-rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B': { | |
| currencyCode: 'USD', | |
| displayName: 'Bitstamp USD', | |
| denominations: [ | |
| { | |
| name: 'USD', | |
| multiplier: '1000000000000000000' | |
| } | |
| ], | |
| networkLocation: { | |
| currency: 'USD', | |
| issuer: 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B' | |
| } | |
| }, | |
| 'EUR-rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B': { | |
| currencyCode: 'EUR', | |
| displayName: 'Bitstamp EUR', | |
| denominations: [ | |
| { | |
| name: 'EUR', | |
| multiplier: '1000000000000000000' | |
| } | |
| ], | |
| networkLocation: { | |
| currency: 'EUR', | |
| issuer: 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B' | |
| } | |
| }, | |
| 'USD-rEn9eRkX25wfGPLysUMAvZ84jAzFNpT5fL': { | |
| currencyCode: 'USD', | |
| displayName: 'Stably USD', | |
| denominations: [ | |
| { | |
| name: 'USD', | |
| multiplier: '1000000000000000000' | |
| } | |
| ], | |
| networkLocation: { | |
| currency: 'USD', | |
| issuer: 'rEn9eRkX25wfGPLysUMAvZ84jAzFNpT5fL' | |
| } | |
| } |
src/common/CurrencyEngine.ts#L348-L359
edge-currency-accountbased/src/common/CurrencyEngine.ts
Lines 348 to 359 in 4a2623c
| } else { | |
| // Find tokenId in allTokensMap that matches this currency code | |
| const tokenId = Object.keys(this.allTokensMap).find( | |
| tid => this.allTokensMap[tid]?.currencyCode === currencyCode | |
| ) | |
| if (tokenId != null) { | |
| newKey = tokenId | |
| } else { | |
| // No matching token found | |
| continue | |
| } |
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.
The LLM makes a valid point - by stopping the loop once we hit the first confirmed transaction, we might miss some still-confirming ones deeper down. Say the user does two spends, and the second one confirms a few blocks before the first one. The second one could reach "confirmed" status while the transaction below is still at 8/10 confirmations or whatever. We'll never dig down to that guy to mark him "confirmed". Not that this doesn't affect completely unconfirmed transactions - those are handled in other places. I think this is a caveat we can live with, but it might be worth noodling on.
4a2623c to
5fcd3b0
Compare
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: Confirmation-only updates never saved
addTransaction decides whether to update an existing tx without considering edgeTransaction.confirmations (or metadata). Changes that only flip confirmations (like RBF replacements setting confirmations: 'dropped') can be ignored, leaving the stored transaction stuck in the prior state.
src/common/CurrencyEngine.ts#L588-L617
edge-currency-accountbased/src/common/CurrencyEngine.ts
Lines 588 to 617 in 5fcd3b0
| this.warn(`addTransaction new tx: ${edgeTransaction.txid}`) | |
| } else { | |
| // Already have this tx in the database. See if anything changed | |
| const transactionsArray = this.transactionList[safeTokenId] | |
| const edgeTx = transactionsArray[idx] | |
| const { otherParams: otherParamsOld = {} } = edgeTx | |
| const { otherParams: otherParamsNew = {} } = edgeTransaction | |
| if ( | |
| // if something in the transaction has changed? | |
| edgeTx.blockHeight < edgeTransaction.blockHeight || | |
| (edgeTx.blockHeight === 0 && edgeTransaction.blockHeight < 0) || | |
| (edgeTx.blockHeight === edgeTransaction.blockHeight && | |
| (edgeTx.networkFee !== edgeTransaction.networkFee || | |
| edgeTx.nativeAmount !== edgeTransaction.nativeAmount || | |
| otherParamsOld.lastSeenTime !== otherParamsNew.lastSeenTime || | |
| edgeTx.date !== edgeTransaction.date)) | |
| ) { | |
| if (edgeTx.date !== edgeTransaction.date) { | |
| needsReSort = true | |
| } | |
| this.warn( | |
| `addTransaction: update ${edgeTransaction.txid} height:${edgeTransaction.blockHeight}` | |
| ) | |
| this.walletLocalDataDirty = true | |
| this.updateTransaction(safeTokenId, edgeTransaction, idx) | |
| } else { | |
| // this.log(sprintf('Old transaction. No Update: %s', tx.hash)) | |
| } | |
| } |
src/ethereum/EthereumEngine.ts#L1858-L1880
edge-currency-accountbased/src/ethereum/EthereumEngine.ts
Lines 1858 to 1880 in 5fcd3b0
| const txid = normalizeAddress(txOtherParams.replacedTxid) | |
| const index = this.findTransaction(edgeTransaction.tokenId, txid) | |
| if (index !== -1) { | |
| const replacedEdgeTransaction = | |
| this.transactionList[edgeTransaction.tokenId ?? ''][index] | |
| // Use the RBF metadata because metadata for replaced transaction is not | |
| // present in edge-currency-accountbased state | |
| const metadata = edgeTransaction.metadata | |
| // Update the transaction's confirmations to dropped | |
| const updatedEdgeTransaction: EdgeTransaction = { | |
| ...replacedEdgeTransaction, | |
| metadata, | |
| confirmations: 'dropped' | |
| } | |
| this.addTransaction( | |
| updatedEdgeTransaction.tokenId, | |
| updatedEdgeTransaction | |
| ) | |
| } |
5fcd3b0 to
0430474
Compare
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
EdgeApp/edge-core-js#690
Description
noneNote
Switches to EdgeTransaction.confirmations, centralizes block-height/confirmation updates, removes legacy dropped-tx polling/unconfirmed counts, and updates engines/tests accordingly.
updateConfirmationsusingEdgeTransaction.confirmations(handles unconfirmed → number → confirmed/dropped; fixes negative heights).updateBlockHeightto set height, recompute confirmations, and emit tx events.numUnconfirmedSpendTxs/lastCheckedTxsDropped; delete related logic and fields; addasMaybeOtherParamsLastSeenTime.updateBlockHeightacrossAlgorand,Cardano,Cosmos,EOS,FIO,Polkadot,Ripple,Stellar,Tezos,Tron,Zano,Zcash,Piratechain,Filecoin, and EVM (EthereumNetwork, FEVMFilfoxAdapter).confirmations: 'dropped'instead ofblockHeight = -1.WalletLocalDataschema (remove unconfirmed tx fields).updateBlockHeight(...)for confirmation/dropped evaluation and align expectations (no-1heights, no unconfirmed counts).EdgeTransaction.confirmationsand related refactors.Written by Cursor Bugbot for commit 0430474. This will update automatically on new commits. Configure here.