asset{btc/ltc}: Add custom wallet constructor for BTC and LTC #2636
Conversation
client/asset/btc/spv_wrapper.go
Outdated
| cl SPVService | ||
| acctNum uint32 | ||
| acctName string | ||
| dir string |
There was a problem hiding this comment.
Can we just populate these during initialization with a single call to AccountInfo instead of calling it every time?
There was a problem hiding this comment.
Uhmm, okay.
EDIT: Some consumers might have an interface for wallet renaming(like Cryptopower), saving this might cause some unexpected problems, I think.
There was a problem hiding this comment.
Yes, thank you.
client/asset/btc/btc.go
Outdated
| if def.Type == availableWallets.Type { | ||
| return fmt.Errorf("(%q): %w", def.Type, asset.ErrWalletTypeAlreadyRegistered) | ||
| } |
There was a problem hiding this comment.
I know you're just adopting the existing pattern from dcr, but this should really be a panic, I think.
There was a problem hiding this comment.
Yh. Currently, the only consumer “might” call this function multiple times but apart from that, I think we can let consumers handle this error anyhow they deem fit.
There was a problem hiding this comment.
I think a panic is appropriate. The consumer can use something like an atomic.Bool to track if the custom type has been registered and avoid repeat registration attempts.
client/asset/ltc/ltc.go
Outdated
| // customSPVWalletConstructors are functions for setting up custom | ||
| // implementations of the btc.BTCWallet interface that may be used by the | ||
| // ExchangeWalletSPV instead of the default spv implementation. | ||
| var customSPVWalletConstructors = map[string]btc.CustomSPVWalletConstructor{} | ||
|
|
||
| // RegisterCustomSPVWallet registers a function that should be used in creating | ||
| // a btc.BTCWallet implementation that the ExchangeWalletSPV will use in place | ||
| // of the default spv wallet implementation. External consumers can use this | ||
| // function to provide alternative btc.BTCWallet implementations, and must do so | ||
| // before attempting to create an ExchangeWalletSPV instance of this type. | ||
| func RegisterCustomSPVWallet(constructor btc.CustomSPVWalletConstructor, def *asset.WalletDefinition) error { | ||
| for _, availableWallets := range WalletInfo.AvailableWallets { | ||
| if def.Type == availableWallets.Type { | ||
| return fmt.Errorf("(%q): %w", def.Type, asset.ErrWalletTypeAlreadyRegistered) | ||
| } | ||
| } | ||
| customSPVWalletConstructors[def.Type] = constructor | ||
| WalletInfo.AvailableWallets = append(WalletInfo.AvailableWallets, def) | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Why does bch use the btc RegisterCustomSPVWallet but ltc has it's own and duplicates the btc constructor code?
There was a problem hiding this comment.
RegisterCustomSPVWallet was not implemented for bch cuz Cryptopower only supports btc, ltc and dcr atm.
ukane-philemon
force-pushed
the
add-custom-wallet-constructor
branch
3 times, most recently
from
2decf1c to
beacdac
Compare
Signed-off-by: Philemon Ukane <[email protected]>
Signed-off-by: Philemon Ukane <[email protected]>
Signed-off-by: Philemon Ukane <[email protected]>
Signed-off-by: Philemon Ukane <[email protected]>
Signed-off-by: Philemon Ukane <[email protected]>
Signed-off-by: Philemon Ukane <[email protected]>
ukane-philemon
force-pushed
the
add-custom-wallet-constructor
branch
from
beacdac to
c615fb9
Compare
There was a problem hiding this comment.
How about just one? buck54321/dcrdex@c615fb9...buck54321:dcrdex:onereg
Thanks for the review and suggestion, I think we still need to register the wallet in ‘WalletInfo.AvailableWallets’ for each asset. |
This a follow-up of https://github.com/decred/dcrdex/pull/1227/files#diff-cd95defa7320b76287f90152b40ee5ec5edec59ca9af2eb9579ecda539ece20a which introduced custom wallet constructors.
These changes are required by the Cryptopower project to provide dex core with a custom built-in wallet.
Closes #2637