Backend/UtxoCoin: support native-segwit with cold storage #254
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
knocte
force-pushed
the
wip/nativeSegwitWithColdStorage
branch
14 times, most recently
from
edfdfa0 to
5496a91
Compare
|
@aarani this is ready for review now, can you have a very quick look? |
aarani
approved these changes
Feb 12, 2024
|
"Many |
|
Actually there's 3 things missing in this PR:
I'll work on the above today. |
knocte
force-pushed
the
wip/nativeSegwitWithColdStorage
branch
11 times, most recently
from
0f4db61 to
dc2850f
Compare
knocte
force-pushed
the
wip/nativeSegwitWithColdStorage
branch
from
7945891 to
4354e22
Compare
This fixes the following crash when trying to broadcast a signed transaction from your cold-storage device: ``` unknown origin account at GWallet.Backend.UtxoCoin.Account.GetSignedTransactionDetails[T](String rawTransaction, Currency currency) in /Users/knocte/Documents/Code/geewalletMASTERclean/src/GWallet.Backend/UtxoCoin/UtxoCoinAccount.fs:line 728 at GWallet.Backend.Account.GetSignedTransactionDetails[T](SignedTransaction`1 signedTransaction) in /Users/knocte/Documents/Code/geewalletMASTERclean/src/GWallet.Backend/Account.fs:line 793 at Program.BroadcastPayment() in /Users/knocte/Documents/Code/geewalletMASTERclean/src/GWallet.Frontend.Console/Program.fs:line 82 at Program.PerformOperation(UInt32 numActiveAccounts, UInt32 numHotAccounts) in /Users/knocte/Documents/Code/geewalletMASTERclean/src/GWallet.Frontend.Console/Program.fs:line 398 at Program.ProgramMainLoop[a]() in /Users/knocte/Documents/Code/geewalletMASTERclean/src/GWallet.Frontend.Console/Program.fs:line 481 at Program.NormalStartWithNoParameters() in /Users/knocte/Documents/Code/geewalletMASTERclean/src/GWallet.Frontend.Console/Program.fs:line 493 ``` This change also reduces a bit of primitive obsession (i.e. string vs BitcoinAddress) to make the code less "stringly-typed" lol. This should have been done in [1] but was an oversight on my part when reviewing. [1] #211
This way, if a version of geewallet that is too old to have native segwit support (or disabled by default) tries to deserialize a transaction proposal, it will error out complaining about version mismatch instead of crashing with a weird error. This commit also tweaks the version comparison, to make sure the VersionMismatch exception is actually raised, and caught by the Frontend (Console, for now).
Their public addresses would then start showing their native segwit address (that starts with "bc1") instead of the nested segwit one. This way they don't need to re-scan the QR code from the coldstorage device. If we don't do this, if you tried to sign-off the transaction on your native-segwit-ready cold storage device, you would get this error: ``` Introduce a file name to load the unsigned transaction: testCold.json Error: The transaction doesn't correspond to any of the accounts in the wallet. Press any key to continue... ``` In case you're wondering if there's any risk that this causes irrecoverable funds when you use this with a coldstorage device that you haven't upgraded yet to NativeSegwit: this could not happen since the JSON format is different, and then deserialization would fail, advising the user to upgrade.
knocte
force-pushed
the
wip/nativeSegwitWithColdStorage
branch
from
ddc8142 to
2d6dccc
Compare
For performance reasons, we don't want to execute migrations everytime we need to iterate over the accounts.
knocte
force-pushed
the
wip/nativeSegwitWithColdStorage
branch
from
02990d0 to
beabb57
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change also reduces a bit of primitive obsession (i.e. string vs BitcoinAddress) to make the code less "stringly-typed" lol.
This should have been done in [1] but was an oversight on my part when reviewing.
[1] #211