Panic when creating Envelope from an ur_string

[danpape]

I upgraded a project of mine from using bc-envelope = "0.18" to bc-envelope = "0.19". (This also happens with "0.21.0")

With "0.18" I was able to create an Envelope from an ur_string representing a envelope containing a seed, but with "0.19" I get the message

called `Option::unwrap()` on a `None` value

which seems to be coming from deep within the bc-ur crate.

(As I was writing this I thought it might be better to file an issue for bc-ur, but since all my client code uses bc-envelope, I'll put it here for now)

To reproduce:

1. use command line seedtool to make a new seed:

> seedtool --version
seedtool-cli 0.1.10
> seedtool -o envelope > small.seed
> cat small.seed
ur:envelope/lftpsogdqdwecartbtvdhysatedtdpotnnrkimpfoyadcsspmosfcnbz

2. If my Cargo.toml depends on bc-envelope = "0.18.2" then I will then depend on bc_ur "0.5.0":

> cargo tree --invert --package bc-ur
bc-ur v0.5.0
├── bc-components v0.13.0
│   └── bc-envelope v0.18.2

and this test function executes just fine:

    #[test]
    fn test_create_envelope_from_ur_string() -> Result<(), Box<dyn std::error::Error>> {
        let seed_file_path = "test/small.seed";
        let seed_envelope_ur = fs::read_to_string(seed_file_path)?;
        let seed_envelope_ur = seed_envelope_ur.trim();
        let _seed_envelope = Envelope::from_ur_string(seed_envelope_ur)?;
        Ok(())
    }

3. When I upgrade bc-envelope to "0.19", or even "0.21" (most recent), then I get bc-ur "0.6.0":

> cargo tree --invert --package bc-ur
bc-ur v0.6.0
├── bc-components v0.14.0
│   └── bc-envelope v0.19.0

> cargo tree --invert --package bc-ur
bc-ur v0.6.0
├── bc-components v0.15.0
│   └── bc-envelope v0.21.0

and then my test function fails with the panic message I showed up above. The relevant part of the stacktrace is:

   5: bc_ur::ur_decodable::URDecodable::from_ur
             at /Users/dpape/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bc-ur-0.6.0/src/ur_decodable.rs:10:32
   6: bc_ur::ur_decodable::URDecodable::from_ur_string
             at /Users/dpape/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bc-ur-0.6.0/src/ur_decodable.rs:15:9
   7: small::tests::test_create_envelope_from_ur_string
             at ./src/small.rs:17:30

I have noticed that in recent versions of bc-ur, the file bc-ur-0.6.0/src/ur_encodable.rs has some if/else code in fn ur(&self) that I locally duplicated in bc-ur-0.6.0/src/ur_decodable.rs in fn from_ur(ur: impl AsRef<UR>). With that patch in place, my test function will fail with this message:

CBOR tag 200 must have a name

I can provide a full test project if you like.

[danpape]

I suppose I should add more details. The patch I made to bc-ur-0.6.0/src/ur_decodable.rs was to change the from_ur function from:

    fn from_ur(ur: impl AsRef<UR>) -> Result<Self> where Self: Sized {
        ur.as_ref().check_type(Self::cbor_tags()[0].name().as_ref().unwrap().clone())?;
        Self::from_untagged_cbor(ur.as_ref().clone().into())
    }

to this:

    fn from_ur(ur: impl AsRef<UR>) -> Result<Self> where Self: Sized {
        let tag = &Self::cbor_tags()[0];
        if let Some(name) = tag.name().as_ref() {
            ur.as_ref().check_type(name.clone())?;
            Self::from_untagged_cbor(ur.as_ref().clone().into())
        } else {
            panic!("CBOR tag {} must have a name", tag.value());
        }
    }

[wolfmcnally]

Hi, Dan!

We recently switched how we're registering CBOR tags in the process to make it easier for third parties to assign new tags (including names, which are used as UR types) and also assign code that creates textual summaries of what tags represent. The upshot of this is that you need to include a call to:

bc_envelope::register_tags();

once at the start of your process, either main() in a full app or in your tests like this:

    #[test]
    fn test_create_envelope_from_ur_string() -> Result<(), Box<dyn std::error::Error>> {
        bc_envelope::register_tags();
        let seed_file_path = "test/small.seed";
        let seed_envelope_ur = fs::read_to_string(seed_file_path)?;
        let seed_envelope_ur = seed_envelope_ur.trim();
        let _seed_envelope = Envelope::from_ur_string(seed_envelope_ur)?;
        Ok(())
    }

It's definitely on me that this isn't better documented. Let me know whether this fixes the problem!

[wolfmcnally]

@danpape I just published bc-ur 0.6.1, which now adds the note:

CBOR tag {} must have a name. Did you call `register_tags()`?

when it throws this error.

[danpape]

Thank you! Yes, adding bc_envelope::register_tags() helps my tests and application. However, I don't see a helpful error message when decoding unless I make a change to bc-ur as I submitted in BlockchainCommons/bc-ur-rust#2 Please check it out and let me know what you think.

[wolfmcnally]

Did you do a cargo update to make sure you're using bc-ur 0.6.1?

[danpape]

Yep, I did do an update and I verified I am now using 0.6.1.

> cargo tree --invert --package bc-ur                        
bc-ur v0.6.1
├── bc-components v0.15.0
│   ├── bc-envelope v0.21.0

If you check BlockchainCommons/bc-ur-rust#2 you should see that PR is adding the register_tags() warning to UR decoding to complement the warning you added for UR encoding in your change for 0.6.1. Perhaps that is not appropriate, but I thought it could help.