refactor: change advice provider to trait

[vlopes11]

This commit introduces a change of the advice provider to be a trait. It also provides a standard implementation MemAdviceProvider.

It splits the initialization of the stack from the inputs of the advice provider.

Pending

• Should we deprecate ProgramInputs?
• Should we change the VM clock to be an internal implementation detail instead of setting it into the advice provider?
• Enable the ignored tests (must be done after the pending topics are resolved)

[bobbinth]

Thank you! I've left a few comments inline - but one general thing: I think this should be just a first PR in a series of 3 - 4 PRs that we'll need to do to refactor this structure. My preference would be to focus this one on introducing the AdviceProvider trait and keep other unrelated changes to a minimum.

[bobbinth]

Thank you! Looks good! I left a few comments inline.

[bobbinth]

Looks good! Thank you! I've added a few more comments inline.

[bobbinth]

nit: I'd change "shouldn't" to "doesn't" here.

[vlopes11]

This is an interesting point of discussion. I often like to use weak imperatives for traits.

Something like: a concrete implementation is won't, and a contract/trait is shouldn't.

The reason for that is the nature of a trait, that is to set guidelines for expected behavior, while these decisions falls ultimately to the implementer. We can of course verbally enforce that, but it will not reflect what happens in the code where the implementer has total control and can easily contradict what is described on the documentation. This means: an implementation of this trait should behave like that, but won't necessarily.

[bobbinth]

Ah - makes sense! I'm fine either way then.

[hackaugusto]

My two cents on this conversation.

The reason for that is the nature of a trait, that is to set guidelines for expected behavior, while these decisions falls ultimately to the implementer.

I don't agree with this point. IMO a trait is not a guideline but a contract. The trait designer is the one that has freedom to decide what is part of the contract, once that is done implementations must follow the contract.

Sometimes the trait is designed to required something. E.g. the From trait must not fail. That is part of the contract, and API users can rely on that, if the implementation fails it is a bug.

We can also have situations were things are unspecified, and this is what we have here when you say operation shouldn't consume the map element. You made a design decision, and the design here is to give no guarantees for the caller. If the caller assumes the key is not removed and uses a loop, the code is most likely buggy, because you allowed for some implementation to remove the key from the map.

Because of that IMO we should either remove the suggestion of using it in a loop, because if one did that, it relies on unspecified behavior. Or change the wording to must, so that this kind of code can be written.

[vlopes11]

I think the From case is mostly an exception, as they add this must not in bold as extra warning note. It is so because this is the stdlib of Rust, and people historically mixed a lot the usage of this trait when instead they should use try_from for fallible cases.

Going from this example of fallible vs infallible, the contract must be expressed on the signature. Meaning:

fn from(arg: T) -> Y is necessarily infallible, and fn try_from(arg: T) -> Result<Y, E> might be fallible.

The contract-level items of a trait must be expressed via signature. If there is no way to do so, then probably the trait itself must be redesigned.

We can take tokio as example. BufReader::get_mut states It is inadvisable to.... We can find the same pattern in other traits. Another example is rand::CryptoRng. This trait doesn't implement anything per se; instead, it just show its users that the RNG implementation is supposed to be cryptographically secure (while no actual guarantee is provided as anyone can simply implement it to any RNG).

[bobbinth]

All looks good! Thank you!