(feat) Refactor Multikey Views and Multisig Views to be extensible.

[dhuseby]

We received a request to add Attributes to both Multikey and Multisig to store non-standard algorithm codecs so that both Multikey and Multisig are extensible. Adding that requires a rethink of the Views approach because the user of Multikey and Multisig will need to supply the impls of the different Views for their experimental keys/sigs. So instead of an abstract factor returning Box<&impl View> we're going to need to move the abstraction further "upstream".

I'm proposing that we switch from abstract factory to a builder trait for building views. That way Multikey and Multisig will depend on a mk::ViewBuilder trait and a ms::ViewBuilder trait respectively when they need to access views.

The primary change is we'll be adding &impl ViewBuilder as a parameter to some functions and all functions that depend on ViewBuilder will be returning Results because constructing a view will now always be fallible (i.e. relies upon try_build to get the concrete View instance).

The existing impls for different algorithms (e.g. Ed25519, etc) will be changed to prove an ViewBuilder impl.

@mikelodder7 @DougAnderson444 thoughts?

[mikelodder7]

The hard part is that each key could potentially have many different configurations and settings so its difficult to encapsulate that in a single trait. I find the easiest and probably most effective way to do this is to have view creation be implementation specific w/o traits and then once the view is created we only interact over traits.

[dhuseby]

I find the easiest and probably most effective way to do this is to have view creation be implementation specific w/o traits and then once the view is created we only interact over traits.

Remember, the codec can be set to anything, not just the fixed set we support right now. When the codec is a "non-standard" codec, the Multikey code needs a way to try construct the requested view with something given to it by the caller. So that can either be a ViewBuilder trait impl or it can be a callback function.

I think there are two parts of the existing design that make the ViewBuilder trait work:

1. If you look at the View impl for Multikey, you'll notice that there are only two Views that take a parameter on creation: CipherView takes the key to use for encryption/decryption, KdfView takes the KDF Multikey which is really just the KDF parameters.
2. All of the parameters are in the Multikey that we're constructing a view on.

What this means is that the ViewBuilder trait is really just converting the View trait into a builder pattern:

trait ViewBuilder<'a> {
    fn conv_vew(&'a self) -> Result<Box<dyn ConvView + 'a>, Error>;
    //...
}

Then the Multikey code can change to be something like:

let must rng = StdRng::from_os_rng();

let mut attrs = mk::Attributes::new();
let key_len: Vec<u8> = Varuint(32).into();
attrs.insert(AttrId::KeyLen, key_len.into());

// impls ViewBuilder trait and knows how to build the views for the custom codec
let vb = MyViewBuilder::new();

// build a multikey with a custom codec and custom attributes
let mk = mk::Builder::new_from_random_bytes(Codec::MyCodecPriv, &mut rng)?
    .with_attributes(&attrs)
    .with_view_builder(&vb)
    .with_comment("test key")
    .try_build()?;

// this call will look at the Multikey::codec value and since it is a custom codec, it
// will call the custom ViewBuilder impl
let conv = mk.conv_view()?;

// this results in a call to MyViewBuilder::conv_view(&self) where &self is &mk
let pk = conv.to_public_key()?;

// the resulting pk is an instance of MyConvView built by MyViewBuilder...

//...

If the Multikey is one of the standard codecs (e.g. Codec::Ed25519Priv) then the caller doesn't have to pass a ViewBuilder instance to the Multikey Builder because the builder will construct the ViewBuilder for the standard instance automagically.

I think this gives enough flexibility and extensibility while also keeping the interface clean. It makes View code—even custom View code—dependent only on the ViewBuilder trait.

[dhuseby]

This is a very minimal refactor now that I think about it. It's just taking the View impl out of Multikey itself and providing impls in each of the codec-specific implementations. Then changing the Multikey struct to have a reference to the ViewBuilder instance and the mk::Builder initialize it.

The only complication I can see is with deserialization. How are we going to get a fully initialized Multikey with a reference to the correct ViewBuilder when we deserialize?

[dhuseby]

How are we going to get a fully initialized Multikey with a reference to the correct ViewBuilder when we deserialize?

To answer myself, I think the correct answer is to have a public interface to the attributes in the Multikey. Then we can just construct instances of trait ViewBuilder that take a reference to the Multikey. That will have to be a fallible operation since the ViewBuilder will be algorithm specific (e.g. Ed25519) and during construction it will check that the codec is correct and all of the required attributes are in the Multikey. This fully decouples the interpretation of the Multikey attributes from the storage of the attributes in the Multikey.