Fixes transaction lists #10
Conversation
There was a problem hiding this comment.
ACK 8d8fe3d
But with a suggestion for improving tests in the future.
src/test.rs
Outdated
| Ok(()) | ||
| } | ||
|
|
||
| async fn electrum_full_scan(wallet: &mut PersistedWallet<Store>) -> anyhow::Result<()> { |
There was a problem hiding this comment.
I'd rather not having unit tests that depend on external network services like mempool.space since the tests will be flaky if mempool.space rate limits tests. Also even though DB tests are more of integration tests, they should be focused as much as possible just on the code being tested plus the local postgres DB.
But if you don't have time to create more self contained tests right now I'm ok with merging this as-is as long as we rework the tests in a future PR.
|
It looks like you need to rebase this PR too. |
|
Hey sorry I had to rebase this and then manually pushed it to the repo master branch. But github got confused since you used your fork |
|
You can see the merged commit here: aae892e |
Previously, if you had a pg instance with multiple wallets and you loaded a wallet and collected all its known transactions youd end up with other txs from other wallets.
This was an issue with the sql which is now fixed.
The tests create three wallets, sync them with electrum, insert into pg, then load them again from pg and have its transactions compared with an unpersisted wallet that gets synced with electrum. They should both match.