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

[Client Refactoring 2] Add a stateless version of execution_transaction #360

Merged
merged 1 commit into from
Feb 4, 2022

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Feb 4, 2022

To make it easier to move execution_transaction to AuthorityAggregator, we first introduce a stateless version of it, and call it from the stateful version. This stateless version will then latter be moved to AuthorityAggregator.
The reason I am doing it this way is to make PRs less intrusive / easier to review, and make the refactoring more smooth. It also allows other people to continue working on the client core.
Most of this PR is straightforwards as most functions called are already stateless.
The only thing I needed to change is to move around the update of local objects. There are two of them, once in the execution_transaction, while the other one in execute_transaction_without_confirmation. The one in execute_transaction_without_confirmation is unnecessary and can be removed.

@lxfind lxfind changed the base branch from main to no-certificates-state February 4, 2022 04:50
@lxfind lxfind force-pushed the no-certificates-state branch from 06e2df1 to 9d11510 Compare February 4, 2022 06:23
@lxfind lxfind force-pushed the make-execution-transaction-stateless branch from 0f00875 to 68ae11c Compare February 4, 2022 06:24
@oxade
Copy link
Contributor

oxade commented Feb 4, 2022

@lxfind nice! Thanks for jumping on this.
You might want to look at some refactoring and modularization work @gdanezis is doing here in case of overlaps/conflicts.
I think there's a lot of similarities and I particular like the MapReduce view in George proposes.
#336

Copy link
Collaborator

@gdanezis gdanezis left a comment

Choose a reason for hiding this comment

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

Yes, the authority API should indeed be useable with &refs, not &mut.

Base automatically changed from no-certificates-state to main February 4, 2022 15:48
@lxfind lxfind force-pushed the make-execution-transaction-stateless branch from 68ae11c to 19a56bd Compare February 4, 2022 15:49
@lxfind lxfind merged commit 936666d into main Feb 4, 2022
@lxfind lxfind deleted the make-execution-transaction-stateless branch February 4, 2022 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants