Implement possible_networks function for Address#618
Implement possible_networks function for Address#618dr-orlovsky merged 1 commit intorust-bitcoin:masterfrom
Conversation
647422a to
742cd8a
Compare
|
Does it make sense to have a In many situations the caller will already know what network it needs, and just wants to do the "ok or not" check. |
742cd8a to
37754f3
Compare
dr-orlovsky
left a comment
There was a problem hiding this comment.
Concept ACK, but the implementation ignores the fact that an address may be created using explicit network data and not parsed. Thus there might be an address with self.network set to Network::signet and this case would not be handled by the proposed implementation
|
If I understand you correctly you'd e.g. want a constructed regtest address to be compatible with testnet? I wonder what an elegant solution would look like. Essentially we are creating a compatibility matrix here. |
37754f3 to
382df19
Compare
|
I hope I implemented what you meant. |
|
I wonder how this function could be tested such that future expansions of the networks enum will lead to failure. Otherwise this function would fail silently by returning false even for addresses of the same network if it's not added to an equivalence class. Maybe we should add a function to |
dr-orlovsky
left a comment
There was a problem hiding this comment.
Yeah, this might cause problems with Network enum extensions. What I propose here is to do the following:
match (self.network, network) {
(a, b) if a == b => true,
(Network::Bitcoin, _) | (_, Network::Bitcoin) => false,
(Network::Testnet, _) | (Network::Regtest, _) | (Network::Signet, _) => true
}
src/util/address.rs
Outdated
There was a problem hiding this comment.
Nit: I think we do not need to introduce this constants since these values can be embedded right into match patterns below.
|
We also really need test vectors here covering all cases |
382df19 to
7143971
Compare
dr-orlovsky
left a comment
There was a problem hiding this comment.
Thank you for updating the code and well-designed test case; there one thing though which I personally unsure (pls see my comment)
src/util/address.rs
Outdated
There was a problem hiding this comment.
Correct me if I'm wrong, but we need to test different segwit versions here, aren't we?
This implies the question if Signet P2TR address is equivalent to Testnet P2TR address, which before the November is not deployed to the network...
There was a problem hiding this comment.
EDIT: Just saw it's only the test, sure, although a bit silly imo.
feel free to ignore the original reply below
P2TR addresses are already valid (or they would be if there wasn't the bech32m problem) and would ideally follow the same conventions as other bech32 addresses. This means we can certainly not say that they aren't compatible, because that might limit interoperability (a wallet doesn't need to be aware of the destination address version's semantics with bech32 addresses and should "just work").
I also think that practically it will be easier to tighten the rules down the road should anything change.
A compromise would be to have three states: compatible, incompatible and unknown. That would allow the user to decide what to do.
7143971 to
94229ae
Compare
|
I added test cases for all witness versions. |
|
Perfect, thank you! |
sanket1729
left a comment
There was a problem hiding this comment.
ACK 94229ae, you don't have to fix the spell error.
| } | ||
|
|
||
| /// Parsed addresses do not always have *one* network. The problem is that legacy testnet, | ||
| /// regtest and signet addresse use the same prefix instead of multiple different ones. When |
There was a problem hiding this comment.
nit (NOT MERGE BLOCKING)
/s/addresse/addresses
There was a problem hiding this comment.
I assume I'll not fix it to not invalidate reviews unless a major change request comes in.
|
Seems to be non-breaking PR with a single commit meeting all the requirements, just introducing a single function + more tests, so will dare to merge it |
Parsed legacy addresses do not always have one network. The problem is that testnet, regtest and signet use the same prefix instead of multiple different ones. When parsing such addresses are always assumed to be testnet addresses. But if one wants to check against if the address belongs to the right network a simple comparison is not enough anymore.