dcSpark / cardano-multiplatform-lib

Rust implementation of Cardano
MIT License
99 stars 36 forks source link

Default byron address network_id to testnet if magic not matched #299

Closed oskin1 closed 9 months ago

oskin1 commented 10 months ago

Fixes Byron(UnknownNetwork(ProtocolMagic(1))) when trying to serialize Byron address from preprod testnet.

rooooooooob commented 10 months ago

when trying to serialize Byron address from preprod testnet.

When does this happen? I'm looking at the serialization code and we don't read the network id since it's fixed and just defers to the byron address impl which has no access to the outer Address struct:

Self::Byron(byron) => {
    use cml_core::serialization::ToBytes;
    buf.extend(byron.to_bytes())
}

and Address::header()

Self::Byron(_) => 0b1000 << 4, // note: no network ID for Byron
rooooooooob commented 10 months ago

Or maybe we need to modify Address::Byron variant to remember bits 3-0 in the header byte? The cddl spec doesn't mention anything about them at all.

; byron addresses:
; bits 7-4: 1000
SebastienGllmt commented 10 months ago

Shelley introduces network IDs in the addresses from 0-15. Byron, on the other hand, does not explicitly have a network ID inside it for mainnet - only for testnet (since Cardano didn't have a testnet at launch an that only came later). IDs for Byron are different though - instead of Network ID, it uses the protocol magic (stuffed in as part of AddrAttributes). These are separate values that serve separate roles

The fact these are separate is a little awkward because for the network_id function on an address it's not clear what should be returned in the Byron address case. The best we can do is look at the protocol magic and then (using a hard-coded lookup table) find the network ID for that protocol magic.

The problem is that the lookup we're doing right now isn't aware of preprod of preview, so it throws an UnknownNetwork error.

There are three ways to tackle this:

  1. We can add preview & preprod to this lookup table
  2. preprod and preview both magic ID and protocol magic were made to be equal to the same value for simplicity. We can assume this will always be the case for future networks (this is what this PR does)
  3. We can choose not to change the code, and instead keep the error and tell recommend not to use this function on Byron addresses and get the network ID some other way (not ideal, but partially unavoidable for things not in the lookup table)
gostkin commented 10 months ago

Shelley introduces network IDs in the addresses from 0-15. Byron, on the other hand, does not explicitly have a network ID inside it for mainnet - only for testnet (since Cardano didn't have a testnet at launch an that only came later). IDs for Byron are different though - instead of Network ID, it uses the protocol magic (stuffed in as part of AddrAttributes). These are separate values that serve separate roles

The fact these are separate is a little awkward because for the network_id function on an address it's not clear what should be returned in the Byron address case. The best we can do is look at the protocol magic and then (using a hard-coded lookup table) find the network ID for that protocol magic.

The problem is that the lookup we're doing right now isn't aware of preprod of preview, so it throws an UnknownNetwork error.

There are three ways to tackle this:

  1. We can add preview & preprod to this lookup table
  2. preprod and preview both magic ID and protocol magic were made to be equal to the same value for simplicity. We can assume this will always be the case for future networks (this is what this PR does)
  3. We can choose not to change the code, and instead keep the error and tell recommend not to use this function on Byron addresses and get the network ID some other way (not ideal, but partially unavoidable for things not in the lookup table)

I'd do it in 2nd way (as done in the PR), since:

  1. so far all the networks are 0 / 1 and if new testnet is added according to these rules we're fine. if we use another option the code still would need a change anyway
  2. the issue with byron is this line https://github.com/dcSpark/cardano-multiplatform-lib/blob/2edabcacafed6af2150dfd82fa3246d1cab0446e/core/rust/src/network.rs#L8, either CML won't work on preprod / preview, or we add new networks, or we just ignore the check
SebastienGllmt commented 10 months ago

Keep in mind we should be added known networks to NetworkInfo anyway (it doesn't contain preview/preprod/sanchonet, but probably it should)

SebastienGllmt commented 9 months ago

We instead implemented #309 which I think is the right approach