Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring attempt for indirect-calls-executor #1155

Merged
merged 6 commits into from
Feb 6, 2023
Merged

Refactoring attempt for indirect-calls-executor #1155

merged 6 commits into from
Feb 6, 2023

Conversation

Kailai-Wang
Copy link
Contributor

This PR tries to do a small refactoring for indirect-calls-executor. The main changes and reasons are listed below:

  • move the content of indirect_calls_executor.rs to lib.rs, as the crate has the same name already: indirect-calls-executor/src/indirect-calls-executor.rs seems unnecessary and overlaps with indirect-calls-executor/src/lib.rs
  • Turn the if .. paradigm to a vector of Executors to more clearly illustrate how the parentchain extrinsics are handled. Otherwise we'll have longer if else branches as more handlers are added, and readability can be significantly reduced.
  • Put each Executor in an individual file
  • We need an extra helper trait DecorateExecutor because Executor has an associated type
  • We try to unify the error handling and pre/post processing: IMHO the successfully handled extrinsics should always be pushed into executed_calls to be further passed into create_processed_parentchain_block_call. In the original code, only shield_funds call is pushed, call_worker not -- we think this might be wrong.

Copy link
Contributor

@clangenb clangenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I think this looks promising, and I see where it is going, but I think the most important thing is not there. The executor should not have hardcoded, which indirect calls it should execute; otherwise, we always have to mess with the executor, when we add a new call that should indirectly be executed.

I have also invested some thoughts about this topic, I invite you to comment on that: #1156.

core/parentchain/indirect-calls-executor/src/lib.rs Outdated Show resolved Hide resolved
core/parentchain/indirect-calls-executor/src/lib.rs Outdated Show resolved Hide resolved
@Kailai-Wang
Copy link
Contributor Author

Thank you @clangenb for the review and suggestions, it makes a lot of sense. I also left a comment in #1156, as mentioned IMHO it doesn't change much in terms of the logic but does have better readability.

The question is do you still want to integrate this PR? If so I can fix the points you mentioned in the review and re-sync it. Otherwise if you don't need it for now -- simply go ahead and close it, it's completely fine :)

@clangenb
Copy link
Contributor

Thanks for asking. I don't have a strong opinion about this PR, I think it is a useful intermediate step, however, the follow-up #1156 will not be the priority. So I would say if it makes your downstream live easier, I am happy to merge the PR after the fixes, otherwise we can just close it.

@Kailai-Wang
Copy link
Contributor Author

if it makes your downstream live easier

It does, one less change is always better :) I'll fix those and request a re-review.

@Kailai-Wang
Copy link
Contributor Author

Updated some files according to the review :)

Copy link
Contributor

@clangenb clangenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for the fixes! Looks good to me now!

@clangenb clangenb added A0-core Affects a core part B1-releasenotes C1-low 📌 Does not elevate a release containing this beyond "low priority" E0-breaksnothing labels Feb 6, 2023
@clangenb clangenb merged commit 73f7724 into integritee-network:master Feb 6, 2023
@Kailai-Wang Kailai-Wang deleted the refactor-indirect-calls-executor branch February 6, 2023 21:36
m-yahya pushed a commit to olisystems/BEST-Energy that referenced this pull request Feb 17, 2023
)

* Refactor indirect_calls_executor

* change licence

* fix based on reviews
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-core Affects a core part B1-releasenotes C1-low 📌 Does not elevate a release containing this beyond "low priority" E0-breaksnothing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants