-
Notifications
You must be signed in to change notification settings - Fork 51
Replace currencyCode with tokenId #1002
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
| const mainnetDenom = this.currencyInfo.denominations.find( | ||
| d => d.name === this.currencyInfo.currencyCode | ||
| ) | ||
| if (mainnetDenom == null) { | ||
| throw new Error( | ||
| `Improbable case where we cannot find the mainnet denom` | ||
| ) | ||
| } | ||
| return mainnetDenom |
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.
I think the universal standard is that denominations[0] is always the main one, so we can skip the search and just use that.
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.
It is, but it isn't enforced anywhere
| this.ethEngine.tokenCheckTransactionsStatus[currencyCode] = 1 | ||
| const tuple: EdgeTransactionsBlockHeightTuple = tokenTxs[currencyCode] | ||
| for (const [tokenId, tuple] of tokenTxs) { | ||
| if (tokenId == null) continue |
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: Main currency transactions skipped in Ethereum network update
The if (tokenId == null) continue statement skips processing the main currency's transactions. The tokenTxs Map is expected to contain an entry for null (main currency), as seen at line 328 where tokenTxs.set(null, ...) is called. Skipping null means main currency transactions won't be added via addTransaction, transaction status won't be updated, and query heights won't be saved for the main currency.
| for (const token of activeTokens) { | ||
| const balanceStatus = this.tokenCheckBalanceStatus[token] ?? 0 | ||
| const txStatus = this.tokenCheckTransactionsStatus[token] ?? 0 | ||
| for (const tokenId of activeTokenIds) { |
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: Main currency excluded from sync progress calculation
The updateOnAddressesChecked function only iterates over enabledTokenIds, which no longer includes null for the main currency. Other places in the codebase correctly use [null, ...this.enabledTokenIds] to include the main currency (e.g., lines 729, 456 in AlgorandEngine, 726 in CosmosEngine). The main currency's balance and transaction status, tracked with tokenId = null, are not included in the sync progress calculation. Additionally, if no tokens are enabled, this causes a division by zero (1 / 0).
| out.newNonce = nonce | ||
| if (out.tokenBal != null) | ||
| out.tokenBal[this.ethEngine.currencyInfo.currencyCode] = balance | ||
| if (out.tokenBal != null) out.tokenBal.set(null, 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: Token balances not stored correctly in Blockbook Map
The code uses bracket notation out.tokenBal[symbol] = balance on a Map object instead of the .set() method. This sets an object property on the Map rather than adding an entry to the map's internal storage. The values won't be retrievable via Map.get(). Additionally, symbol is a currency code, but the Map is now keyed by EdgeTokenId (tokenId). The main currency line at 103 was correctly updated to use .set(null, balance), but this token balance line was not updated to match.
They'll use empty string as the null key to maintain JSON compatibility
a836714 to
2378b55
Compare
Use empty string for null to maintain JSON compatibility for disk-cached data. We can use Map for ephemeral properties.
2378b55 to
a75e721
Compare
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
EdgeApp/edge-core-js#688
Description
noneNote
Refactors wallet engine APIs to use tokenId (null for mainnet) instead of currencyCode for balances, transactions, and sync state, updating all engines, Ethereum adapters, and tests.
currencyCode-keyed data withtokenId(usenullfor mainnet); persistnullas empty string in JSON.getCurrencyCode(tokenId)andgetDenomination(tokenId)helpers.updateBalance(tokenId, ...),addTransaction(tokenId, ...),getBalance({ tokenId }),findTransaction(tokenId, ...), etc.Map<EdgeTokenId, number>.tokenId.tokenId = null; adapt denomination/currency code retrieval.tokenId.tokenId; use Maps fortokenBal/tokenTxs.forWhichTokenId.pluginIdin logs.tokenId(with empty-string key for mainnet) and updated method signatures.Written by Cursor Bugbot for commit a75e721. This will update automatically on new commits. Configure here.