Electrum sp refactors #1781
Electrum sp refactors #1781
Conversation
rafael-xmr
force-pushed
the
electrum-sp-refactors
branch
from
927b639
to
a3e131d
Compare
* Enhance the code for sending/sending-ALL for Electrum * remove print statements [skip ci] * update bitcoin base and minor reformatting
…trum-sp-refactors
|
|
||
| factory BitcoinAddressRecord.fromJSON(String jsonSource, {BasedUtxoNetwork? network}) { | ||
| this.scriptHash = scriptHash ?? BitcoinAddressUtils.scriptHash(address, network: network!); |
There was a problem hiding this comment.
if we are sure that network won't be null, then shouldn't we make it required?
you are not setting it in the fromJson function so it will be null
| decoded['address'] as String, | ||
| index: decoded['index'] as int, | ||
| isHidden: decoded['isHidden'] as bool? ?? false, | ||
| labelIndex: decoded['labelIndex'] as int, |
There was a problem hiding this comment.
this won't be backward compatible
| network: (decoded['network'] as String?) == null | ||
| ? network | ||
| : BasedUtxoNetwork.fromName(decoded['network'] as String), | ||
| silentPaymentTweak: decoded['silent_payment_tweak'] as String?, |
There was a problem hiding this comment.
why are we no longer saving the tweak? isn't it needed for sending
| final derivationPath = "m/84'/0'/$i'"; | ||
| final xpub = | ||
| await bitcoinLedgerApp.getXPubKey(derivationPath: derivationPath); | ||
| Bip32Slip10Secp256k1 hd = | ||
| Bip32Slip10Secp256k1.fromExtendedKey(xpub).childKey(Bip32KeyIndex(0)); | ||
|
|
||
| final address = generateP2WPKHAddress( | ||
| hd: hd, index: 0, network: BitcoinNetwork.mainnet); | ||
| final xpub = await bitcoinLedgerApp.getXPubKey(derivationPath: derivationPath); | ||
| final hd = Bip32Slip10Secp256k1.fromExtendedKey(xpub) | ||
| .childKey(Bip32KeyIndex(0)) | ||
| .childKey(Bip32KeyIndex(index)); |
There was a problem hiding this comment.
need to double check this with you and Konstantin
There was a problem hiding this comment.
wouldn't it be better if we unify the BitcoinTransactionPriority with the ElectrumTransactionPriority?
i.e have a single model that we map both to it, so we only use one model and not have checks on which model we should be using
plus, like this which one would we show the values of?
I noticed that you keep using ElectrumTransactionPriority everywhere anyway, so there will be some edge cases and stuff, so let's just unify them into 1 model at the time of fetching (i.e in _handleGetFeeRates in the worker) and use that unified model everywhere
| required this.silentPaymentTweak, | ||
| required this.silentPaymentLabel, |
There was a problem hiding this comment.
do you not need to save them anymore?
or are you saving them somewhere else
| Box<WalletInfo> walletInfoSource, | ||
| Box<UnspentCoinsInfo> unspentCoinSource, | ||
| bool isDirect, | ||
| bool mempoolAPIEnabled, |
There was a problem hiding this comment.
why do we need this to be passed to the wallet service? why not just get it from the shared prefs, so that if it's value changes we get the latest update
that's for all 3 electrum wallets (btc, ltc, bch)
Issue Number (if Applicable): Fixes #
Description
Please include a summary of the changes and which issue is fixed / feature is added.
Pull Request - Checklist