Prepare for non-Base58 addresses [Step 1] #1593
Conversation
furszy
force-pushed
the
2020_refactor_addresses
branch
2 times, most recently
from
c8f0f9e to
f45da49
Compare
|
Rebased + rpc files CBitcoinAddress migration added. |
random-zebra
left a comment
random-zebra
left a comment
There was a problem hiding this comment.
Big PR. Good job 👍 We can (should) probably simplify and abstract some of the ugliness due to staking addresses with a better definition of them (either as variant or child class) in the future.
Anyway, looking good from visual inspection: concept and code ACK modulo some questions, comments and/or nits inlined.
Mostly around:
-
IsValidDestinationStringformula for staking addresses with&& -
The extra overridden version of
IsValidDestinationString: we don't need params to be passed as an external argument toIsValid(at the moment). We should probably include that only when we remove the dependency fromCChainParams -
The usage of
!boost::get<CNoDestination>(&address)instead ofIsValidDestination(address). There are many instances, I haven't commented all of them. -
a few double decoding operations due to the general path
if (!IsValidDestinationString(address_string) { // return some error } CTxDestination dest = DecodeDestination(address_string); ...which should be
CTxDestination dest = DecodeDestination(address_string); if (!IsValidDestination(dest)) { // return some error } ...
This patch removes the need for the intermediary Base58 type
CBitcoinAddress, by providing {Encode,Decode,IsValid}Destination
function that directly operate on the conversion between strings
and CTxDestination.
-- Not all of the CBitcoinAddress instances were ported --
Coming from btc@5c8ff0d448ffdc6340b195ddfa2128d5f21a839b
furszy
force-pushed
the
2020_refactor_addresses
branch
from
f45da49 to
968d366
Compare
|
Thanks for the feedback zebra, went commit by commit and solved most of them. |
furszy
force-pushed
the
2020_refactor_addresses
branch
2 times, most recently
from
60966f6 to
c6a3e91
Compare
|
just force pushed it few times because found more boost::get to move to IsValidDestination. It's good now. |
furszy
force-pushed
the
2020_refactor_addresses
branch
2 times, most recently
from
40557f1 to
024d01c
Compare
GUI missing fixes
furszy
force-pushed
the
2020_refactor_addresses
branch
from
024d01c to
ac99512
Compare
422d2d3 CBitcoinAddress to wrapper migration (furszy) 287a7b0 CompactTallyItem unused struct removed (furszy) 4b4c5a0 Replace CBitcoinSecret with {Encode,Decode}Secret (furszy) Pull request description: Mixed few things here: 1) Follow up over #1593 work. The only remaining places that are not using the destination methods are in the walletmodel and wallet objects (next PR), the rest were all already migrated. 2) CBitcoinSecret with {Encode,Decode}Secret (adapted version of upstream@32e69fa0). 3) Unused code removal (second commit). ACKs for top commit: random-zebra: ACK 422d2d3 Fuzzbawls: ACK 422d2d3 Tree-SHA512: de45c2d66bdb76f48be33352fb278d91294e0171cb1af5f677c5393b7beb8441640aeaf714bc01d3d576e635f3521969b122fb2ab1c7f7aff5cb0c1aeb469b09
3 participants
This patch removes the need for the intermediary Base58 type CBitcoinAddress, by providing {Encode,Decode,IsValid} Destination functions that directly operate on the conversion between std::strings and CTxDestination.
As a side, it also fixes a number of indentation issues, and removes probably several unnecessary implicit CTxDestination<->CBitcoinAddress conversions.
It's not a small change but it will give us much more flexibility once it's fully done over every current and new address type.
The first commit is coming from upstream partially (11117), the rest is purely tackling our code (we have so so many more places using the CBitcoinAddress class).
And finally, have implemented a second abstraction (
Destinationwrapper) on top of the base58 address to {Encode,Decode,IsValid} Staking addresses as well.Note:
This migration is not fully complete. It's first step, we still have places using the CBitcoinAddress object instead of the CTxDestination abstraction.