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
[r2r] Remove outdated transactions from transaction history #1328
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.
Thanks for the PR!
It's a first review. @caglaryucekaya Please also check if the same should be done for my_tx_history_v2
.
…dated transactions
mm2src/coins/utxo/utxo_common.rs
Outdated
@@ -2056,7 +2056,8 @@ where | |||
}; | |||
let mut history_map: HashMap<H256Json, TransactionDetails> = history | |||
.into_iter() | |||
.map(|tx| (H256Json::from(tx.tx_hash.as_bytes()), tx)) | |||
.filter(|tx| H256Json::from_str(&tx.tx_hash).is_ok()) | |||
.map(|tx| (H256Json::from_str(&tx.tx_hash).unwrap(), tx)) |
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.
There was an unexpected bug here at the creation of the history_map. Using H256Json::from(tx.tx_hash.as_bytes())
created a different hash than the hashes inside the tx_ids
vector. As a result, existing transactions in the history_map
were ignored and all the transactions in the tx_ids
were inserted repeatedly into the history_map
. Creating the H256 by H256Json::from_str(&tx.tx_hash)
fixed the problem.
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.
Good catch :)
…move-outdated-txs
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.
🔥
@sergeyboyko0791 Could you please take a look too? :) |
If everything is fine, I can apply the same change to other coins as well before merging the PR |
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.
LGTM!
I think we have the same bug in QRC20, and we also need to remove outdated transactions from from QRC20 tx history.
|
d01a4f9
QRC20 didn't have the same bug, but tx history v2 did so I also fixed it |
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.
One minor change.
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.
👍
Fixes #1321 for UTXO coins by removing outdated transactions from transaction history