-
Notifications
You must be signed in to change notification settings - Fork 53
Track token-loaded state more explicitly #671
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
dae8857
to
6a3e63b
Compare
6a3e63b
to
b553d95
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: Incorrect Token Removal Detection
The onNewTokens
callback incorrectly logs "removed" tokens. The removed
variable is calculated as whatsNew(detectedTokenIds, tokenIds)
. Since tokenIds
represents only the newly detected tokens from the current callback invocation, and detectedTokenIds
represents all currently detected tokens, this calculation incorrectly identifies existing detected tokens (those not in the current new batch) as "removed." This is misleading as onNewTokens
is an add-only callback, and no tokens are actually removed from the wallet's detected list. The variable name removed
is therefore inaccurate and confusing.
src/core/currency/wallet/currency-wallet-callbacks.ts#L142-L147
edge-core-js/src/core/currency/wallet/currency-wallet-callbacks.ts
Lines 142 to 147 in b553d95
// Logging: | |
const added = whatsNew(tokenIds, detectedTokenIds) | |
const removed = whatsNew(detectedTokenIds, tokenIds) | |
const shortId = walletId.slice(0, 2) | |
input.props.log.warn( | |
`enabledTokenIds: ${shortId} engine detected tokens, add [${added}], remove [${removed}]` |
Was this report helpful? Give feedback by reacting with 👍 or 👎
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
none