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.

Sign up for GitHub

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

Jump to bottom

A0-2883: Make keychain synchronous #313

Merged
merged 7 commits into from
Jul 27, 2023
Merged

A0-2883: Make keychain synchronous #313

merged 7 commits into from
Jul 27, 2023

Conversation

woocash2
Copy link
Contributor

@woocash2 woocash2 commented Jul 25, 2023 •
edited
Loading

The general change was to make sign(...) in crypto/signature::Keychain synchronous.
Removal of 1 async keyword caused all the other changes... The other changes included:

  • removal of async from other functions
  • removal of await in function calls which were no longer async
  • changing #[tokio::test] to #[test] if a test was no longer async
  • removal of one #[async_trait]
  • possibly some other minor stuff.

@github-actions
Copy link

Please make sure the following happened

  • Appropriate tests created
  • Infrastructure updated accordingly
  • Updated existing documentation
  • New documentation created
  • Version bumped if breaking changes

ggawryal
ggawryal reviewed Jul 25, 2023
View reviewed changes
Copy link
Contributor

@ggawryal ggawryal left a comment

Choose a reason for hiding this comment

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

Good job, left minor comments.

crypto/src/signature.rs Show resolved Hide resolved
consensus/src/testing/byzantine.rs Show resolved Hide resolved
crypto/src/signature.rs Show resolved Hide resolved
mock/src/network.rs Show resolved Hide resolved
@woocash2 woocash2 requested a review from ggawryal July 26, 2023 09:46
ggawryal
ggawryal approved these changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@ggawryal ggawryal left a comment

Choose a reason for hiding this comment

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

LGTM

kostekIV
kostekIV reviewed Jul 27, 2023
View reviewed changes
Copy link
Contributor

@kostekIV kostekIV left a comment

Choose a reason for hiding this comment

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

pls also change the keychain signature in the docs/src/aleph_bft_api.md

consensus/src/runway/mod.rs Show resolved Hide resolved
@woocash2 woocash2 merged commit f79ee23 into main Jul 27, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fixxxedpoint fixxxedpoint fixxxedpoint approved these changes

@ggawryal ggawryal ggawryal approved these changes

@kostekIV kostekIV kostekIV approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@woocash2 @kostekIV @ggawryal @fixxxedpoint