feat: allow host to be implemented on custom context #2112
Conversation
ghost
changed the title
CodSpeed Performance ReportMerging #2112 will not alter performanceComparing Summary
|
rakita
left a comment
rakita
left a comment
There was a problem hiding this comment.
I do see that the blanked Host is not a good approach, but imo I would remove Host trait from the repo and directly use ContextTr functions inside instructions.
|
i quite liked the fact the interpreter instructions only know the Host interface, but putting it in ContextTr would allow a similar behavior to Handler so it makes sense, ill modify the PR |
ghost
requested a review
from
| fn error(&mut self) -> &mut Result<(), <Self::Db as Database>::Error>; | ||
| fn tx_journal(&mut self) -> (&mut Self::Tx, &mut Self::Journal); | ||
| // default implementationHost calls interface | ||
| fn set_error(&mut self, error: <Self::Db as Database>::Error) { |
There was a problem hiding this comment.
It would be better to remove these functions and use old context in places where for example set_error was called.
They are all one line calls, and block_hash as a more complex function is used only in one place
There was a problem hiding this comment.
i have removed set_error which is redundant, not sure i got what you mean in regards to removing the functions, if you mean that interpreter should get the journal itself and set the context error i think it just adds code to the instructions themselves, and imo interpreter shouldn't set the DB error of context so these functions make sense (maybe besides tload tstore which only proxy to journal), personally i like the fact i can hook host calls before they get to the journal by only implementing context, removing these will force me to implement journal which is a lot more work than contextTr
There was a problem hiding this comment.
Host already required ContextTr. In next PR if there is no host i could remove error field from Context and put it inside Interpreter, that error definetely looks strange, and this would allow Interpreter to return Action or Error.
Journal is kinda bloated rn and part of instructions are leaking inside Frame if all of it is contained inside instruction things become simpler.
If this is done you could still intercept journal calls but you would have few fn sstore(..) { self.inner.sstore(..) } lines
ghost
closed this
|
hey @omer-gen this is good PR, is it okay for me to merge it and I will do follow up? |
|
Yeah feel free to, I submitted it to help, but I couldn't complete the entire change you had in mind all the way through |
That is okay, I can take over. Thanks for contributing! |
2 participants
The current blanket implementation of ContextTr prevents users from implementing their custom host, with this PR it is now possible to opt in for the blanket implementation using a marker, it also explicitly trait bounds Host which is mandatory for the current flow to work