-
Notifications
You must be signed in to change notification settings - Fork 39
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
Fix bunch of bugs #363
Fix bunch of bugs #363
Conversation
✅ Deploy Preview for cheery-moxie-4f1121 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
tACK 080ff07 - wallet is actually usable again
return this.getOwnedVout(tx).reduce((acc, u) => acc + u?.value ?? 0, 0); | ||
const txid = tx.txid; | ||
|
||
return tx.vout | ||
.filter( | ||
(_, i) => | ||
this.getOutpointStatus( | ||
new COutpoint({ | ||
txid, | ||
n: i, | ||
}) | ||
) & OutpointState.OURS | ||
) | ||
.reduce((acc, u) => acc + u?.value ?? 0, 0); |
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.
This adds quite a bit of extra code whenever we need to aggregate any kind of balance / script type / etc, are you sure we should remove the getOwnedX
utility functions?
I would not reject the PR for it, but I prefer the old method.
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 don't think they will have any applications outside getDebit
and getCredit
, that's why I removed them.
I'd say we re-add them if they become useful again?
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.
A-OK with me, was just my personal comment, no blocker.
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.
tACK 93024c2
Tested a few random TXs and "rolling" Synchronisation quickly, still looks good.
Abstract
Fix many bugs that I have accidentally added in the latest PRs
First commit: cherry-pick Duddino's commit from #354 that fixes the bug ".isLocked is not a function"
Second commit: due to bad merge
getLatestBlocks
was returning early on non-shield walletsThird commit: make sure that the
blockheight
is updated when toggling networkFourth and fifth commits: The mempool function
ownTransaction
was broken... So I fixed it, moved the implementation to the wallet class and removed the useless functionsgetOwnedVin
andgetOwnedVout
Testing
Test that you can create txs and the wallet syncs new transactions when new block arrives