Introduce Host object

[frisitano]

This PR introduces a Host trait as described in #1073 with a default implementation provided by the DefaultHost.

[bobbinth]

Thank you! Looks good! I left some comments inline - mostly about simplifying the structure a bit and renaming a few method.

Also, let's merge #1085 into this as I think that's the way to go and it would make it simpler to review the changes more holistically.

[bobbinth]

One other thought: maybe it makes sense to rename AdviceInjector into AdviceRequest and use it as a single enum for both AdviceInjector's and AdviceExtractor's.

Update: though, maybe let's not do this for now - maybe we can do it as a next step.

[bobbinth]

Thank you! Looks good! I left a few more comments inline - mostly about improving ergonomics of the code.

[bobbinth]

Another thing I'm wondering is whether it would make sense to split Host::handle_request() into 3 separate methods? For example, the trait could look like so:

pub trait Host {
    // REQUIRED METHODS
    // --------------------------------------------------------------------------------------------

    fn get_advice<S: ProcessState>(
        &mut self,
        process: &S,
        extractor: AdviceExtractor,
    ) -> Result<HostResponse, ExecutionError>;

    fn set_advice<S: ProcessState>(
        &mut self,
        process: &S,
        injector: AdviceInjector,
    ) -> Result<HostResponse, ExecutionError>;

    // PROVIDED METHODS
    // --------------------------------------------------------------------------------------------

    fn on_debug<S: ProcessState>(&self, process: &S, options: &DebugOptions) {
        debug::print_debug_info(process, options);
    }

    fn pop_adv_stack<S: ProcessState>(&mut self, process: &S) -> Result<Felt, ExecutionError> {
        self.get_advice(process, AdviceExtractor::PopStack).map(|res| res.into())
    }

    // TODO: other extractor wrappers

From the implementer's standpoint, I don't think this is any more complicated than the current approach (and maybe slightly simpler), and I think it is cleaner overall. Would be even cleaner if UpdateMerkleNode didn't return any values - but not sure if we can do anything about it.

[frisitano]

Another thing I'm wondering is whether it would make sense to split Host::handle_request() into 3 separate methods? For example, the trait could look like so:

pub trait Host {
    // REQUIRED METHODS
    // --------------------------------------------------------------------------------------------

    fn get_advice<S: ProcessState>(
        &mut self,
        process: &S,
        extractor: AdviceExtractor,
    ) -> Result<HostResponse, ExecutionError>;

    fn set_advice<S: ProcessState>(
        &mut self,
        process: &S,
        injector: AdviceInjector,
    ) -> Result<HostResponse, ExecutionError>;

    // PROVIDED METHODS
    // --------------------------------------------------------------------------------------------

    fn on_debug<S: ProcessState>(&self, process: &S, options: &DebugOptions) {
        debug::print_debug_info(process, options);
    }

    fn pop_adv_stack<S: ProcessState>(&mut self, process: &S) -> Result<Felt, ExecutionError> {
        self.get_advice(process, AdviceExtractor::PopStack).map(|res| res.into())
    }

    // TODO: other extractor wrappers

From the implementer's standpoint, I don't think this is any more complicated than the current approach (and maybe slightly simpler), and I think it is cleaner overall. Would be even cleaner if UpdateMerkleNode didn't return any values - but not sure if we can do anything about it.

I think this has implications for encapsulation and extensibility. We are removing a level of indirection / encapsulation (removal of the HostRequest layer) which means the Host now has to be aware of the different variants of the HostRequest enum. Additionally if we want to introduce a new variant of the HostRequest enum we would have to modify the Host trait to support it (reduced extensibility). However maybe this is fine, what do you think?

[bobbinth]

I think this has implications for encapsulation and extensibility. We are removing a level of indirection / encapsulation (removal of the HostRequest layer) which means the Host now has to be aware of the different variants of the HostRequest enum. Additionally if we want to introduce a new variant of the HostRequest enum we would have to modify the Host trait to support it (reduced extensibility). However maybe this is fine, what do you think?

I agree that there is a tradeoff here - but I think it is probably OK to make it. My thinking is that we probably won't introduce top-level variants of HostRequest enum too frequently (I'm thinking we might add something like on_event() method, but not sure what beyond that). But also, even with having a single handle_request(), changing HostRequest enum would probably result in some amount of downstream changes.

[bobbinth]

Looks good! Thank you! I left a few small comments inline. Once these are addressed, we can merge.