Default to native segwit change addresses for testnet #837
Conversation
| @@ -1625,9 +1625,16 @@ export class Wallet { | |||
| const self = this; | |||
| return co<PrebuildTransactionResult>(function *() { | |||
| // Whitelist params to build tx | |||
| const whitelistedParams = _.pick(params, self.prebuildWhitelistedParams()); | |||
| let whitelistedParams = _.pick(params, self.prebuildWhitelistedParams()); | |||
There was a problem hiding this comment.
should the default be set in prebuildWhitelistedParams() perhaps? we leak coin-specific code here that could be avoided
There was a problem hiding this comment.
It's a good idea, but I think this just returns the property names to take from the params object and can't actually set any defaults. Would be good to not have coin specific code in here, but it would require a bit of refactoring.
There was a problem hiding this comment.
ah yes of course
weird that we made it this far without having a func providing coin-specific defaults here
let's not refactor in this PR imo
| @@ -1625,9 +1625,16 @@ export class Wallet { | |||
| const self = this; | |||
| return co<PrebuildTransactionResult>(function *() { | |||
| // Whitelist params to build tx | |||
| const whitelistedParams = _.pick(params, self.prebuildWhitelistedParams()); | |||
| let whitelistedParams = _.pick(params, self.prebuildWhitelistedParams()); | |||
There was a problem hiding this comment.
It's a good idea, but I think this just returns the property names to take from the params object and can't actually set any defaults. Would be good to not have coin specific code in here, but it would require a bit of refactoring.
| * Indicates whether a coin defaults to native segwit change outputs | ||
| * @returns {boolean} | ||
| */ | ||
| defaultsToP2wshChange() { |
There was a problem hiding this comment.
Maybe call this defaultChangeAddressType() which returns an element of some UtxoAddressType enum? I'm not sure if we already have an enum like that, but if not I'd be fine with defining one.
There was a problem hiding this comment.
I would prefer to only send an addressType parameter in the case that the SDK requests a different format than the general address default. In other cases, I would like to continue to leave it up to platform.
If the `addressType` is not specified explicitly by the user, we will default to native segwit (p2wsh) addresses for change outputs on tbtc and tltc. After a period of testing, we will enable the same for btc and ltc. Ticket: BG-23508
murchandamus
force-pushed
the
BG-23508.defaultToNativeSegwitChange
branch
from
ed0e690
to
64d2a15
Compare
There was a problem hiding this comment.
Maybe call this defaultChangeAddressType() which returns an element of some UtxoAddressType enum? I'm not sure if we already have an enum like that, but if not I'd be fine with defining one.
Renamed to defaultChangeAddressType(). I have not introduced a UtxoAddressType enum, yet. I would tend to use the types already defined in bitgo-utxo-lib to that end, would that be good?
I want to only send an addressType parameter in the case that the SDK requests a different format than the general address default. In other cases, I would like to continue to leave it up to platform, hence I'm not always setting the value defined in defaultChangeAddressType(). This was why I previously went with defaultsToP2wshChange() to make that more explicit. In the long run, I'd hope native segwit (or p2tr) would be the default, but I guess stuff like that always takes longer than expected. What's a good enough approach for a long-term temporary override? ;)
| * Indicates whether a coin defaults to native segwit change outputs | ||
| * @returns {boolean} | ||
| */ | ||
| defaultsToP2wshChange() { |
There was a problem hiding this comment.
I would prefer to only send an addressType parameter in the case that the SDK requests a different format than the general address default. In other cases, I would like to continue to leave it up to platform.
I don't think so, |
| @@ -22,4 +22,8 @@ export class Tbtc extends Btc { | |||
| getFullName() { | |||
| return 'Testnet Bitcoin'; | |||
| } | |||
|
|
|||
| defaultChangeAddressType() { | |||
| return this.supportsP2wsh() ? `p2wsh` : this.supportsP2shP2wsh() ? `p2shP2wsh` : `p2sh`; | |||
There was a problem hiding this comment.
return "p2wsh"; would be cleaner here (the commit message suggests that it is equivalent)
|
Upon further conferring with Tyler, we are moving the default native segwit change address functionality to platform. We will gate per SDK version there. |
Ticket: BG-23508