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

Introduce VerificationCodeSubsystem #4121

Merged
merged 4 commits into from
Jul 8, 2024

Conversation

akshaymankar
Copy link
Member

@akshaymankar akshaymankar commented Jul 3, 2024

https://wearezeta.atlassian.net/browse/WPB-8891

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@echoes-hq echoes-hq bot added echoes: technical-roadmap/throughput More specific category, to highlight task aiming at improving the development velocity and effici... echoes: technical-roadmap/technical-debt More specific category, to highlight Technical Debt being tackled. labels Jul 3, 2024
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jul 3, 2024
@akshaymankar akshaymankar force-pushed the wpb-8891/verification-code-subsystem branch 6 times, most recently from a2fab76 to 287a89e Compare July 4, 2024 09:22
@akshaymankar akshaymankar force-pushed the wpb-8891/verification-code-subsystem branch from 287a89e to bb775f6 Compare July 4, 2024 09:25
@akshaymankar akshaymankar marked this pull request as ready for review July 4, 2024 17:56
Copy link
Contributor

@pcapriotti pcapriotti left a comment

Choose a reason for hiding this comment

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

LGTM

defaulGen :: StdGen
defaulGen = mkStdGen 0xBAD

withStatefulGen :: (Member (State StdGen) r) => (StdGen -> (a, StdGen)) -> Sem r a
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be generalised to any type s instead of StdGen.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was wondering why there is no modify, but thought probably because of lack of atomicity guarantees. So I decided to write a specific one. So I didn't bother generalizing it in this module.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean? There is a modify, but this one is an analogue of the StateT constructor. As a rule of thumb, I think it's good to turn types like StdGen above into type variables, because that limits the ways they could be used incorrectly in the implementation. Not a huge concern here though, obviously.

@akshaymankar akshaymankar merged commit a497b48 into develop Jul 8, 2024
10 checks passed
@akshaymankar akshaymankar deleted the wpb-8891/verification-code-subsystem branch July 8, 2024 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: technical-roadmap/technical-debt More specific category, to highlight Technical Debt being tackled. echoes: technical-roadmap/throughput More specific category, to highlight task aiming at improving the development velocity and effici... ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants