Try constructor improvements for Delegations/ delegated roles

[jku]

Currently creating e.g. succinct delegations looks like this

BIT_LENGTH = int(math.log2(NUMBER_OF_BINS))
succinct_roles = SuccinctRoles(
    keyids=[bins_key.keyid],
    threshold=THRESHOLD,
    bit_length=BIT_LENGTH,
    name_prefix=NAME_PREFIX,
)
delegations_keys_info: Dict[str, Key] = {}
delegations_keys_info[bins_key.keyid] = bins_key

targets.signed.delegations = Delegations(
    delegations_keys_info, roles=None, succinct_roles=succinct_roles
)

Looks bad, i'm sure we could get that to something a bit like:

targets.signed.delegations = Delegations(SuccinctRoles.new(NUMBER_OF_BINS, NAME_PREFIX))
targets.add_key(bins_key)

Or alternatively

targets.signed.delegations = Delegations.new_succint(NUMBER_OF_BINS, NAME_PREFIX)
targets.add_key(bins_key)

What makes sense probably depends on what the other delegation types (most importantly paths delegation) will look like...

[MVrachev]

If we use the first approach:

targets.signed.delegations = Delegations(SuccinctRoles.new(NUMBER_OF_BINS, NAME_PREFIX))
targets.add_key(bins_key)

we would have to:

1. make Delegations.keys an optional attribute in Delegations.__init__
2. add a new method SuccinctRoles.new(NUMBER_OF_BINS, NAME_PREFIX))

If we use the second approach:

targets.signed.delegations = Delegations.new_succint(NUMBER_OF_BINS, NAME_PREFIX)
targets.add_key(bins_key)

we would have to:

1. make a new method Delegations.new_succint(NUMBER_OF_BINS, NAME_PREFIX)
2. make all SuccinctRoles attributes optional in SuccinctRoles.__init__ or add a new method as a replacement for SuccinctRoles.__init__ where keyids attribute can be skipped.

There is a third option I just came up with:

targets.signed.delegations = Delegations(SuccinctRoles(NUMBER_OF_BINS, NAME_PREFIX)
targets.add_key(bins_key)

we would have to:

1. make Delegations.keys an optional attribute in Delegations.__init__
2. make all SuccinctRoles attributes optional in SuccinctRoles.__init__ as if we don't want to change the order of the arguments we would have to make all of them optional.

I think I am in favor of the third option as I don't like the idea of adding new methods and also it seems the simplest to use.
I will make a small prototype.

PS: I was thinking if we can remove the targets.add_key(bins_key) step and leave those new methods
to generate a new key for us as this is most likely going to be used in testing, but then I realized that with this simple syntax we encourage our users to rely on us to generate keys for them.
That would not be good for us as we would have to make sure that the default algorithm for the key should be secure and non-vulnerable.

PS2: I considered moving keyids and threshold arguments to be the last arguments in SuccinctRoles.__init__ this way the second and third option will require only making keyids and threshold as optional, but that will be a breaking change.

[MVrachev]

After I tried implementing the third option I realized that the end result was complicated as actually I had to initialize delegations like this:

targets.signed.delegations = Delegations(
         succinct_roles=SuccinctRoles(
                 bit_length=NUMBER_OF_BINS, name_prefix=NAME_PREFIX
         )
)
targets.add_key(bins_key)

It required 4 lines just to initialize the targets.delegations as we use black.

Then, I tried the first option as it seemed like a good balance - we were creating a new method, yes, but inside SuccinctRoles which is a helper class and it doesn't matter so much.
Here is the prototype: MVrachev@0405e9d

[jku]

Thanks that was useful.

Looking at this again now, I don't think pushing these ergonomy helpers deep down the hierarchy is good: users don't want to dig through Metadata docs, then Targets docs, then through Delegations docs just to find "SuccinctRoles" -- they just want to create Metadata with succinct delegations. Helper classes should be implementation details as much as possible.

I think we should move helpers as high in the chain as it makes sense: I think in this case it's Delegations (because Targets contains both delegations and targetfiles so we should tell users to create a new Targets to change delegations), so:

targets.signed.delegations = Delegations.new_succinct(16, "prefix")

or during construction

delegations = Delegations.new_succinct(16, "prefix")
md = Metadata(Targets(expires=expires, delegations=delegations))

it's still pretty ugly and undiscoverable but I'd say better