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

Architecture: use newtype pattern #119

Open
kos-for-juspay opened this issue Dec 12, 2022 · 0 comments
Open

Architecture: use newtype pattern #119

kos-for-juspay opened this issue Dec 12, 2022 · 0 comments
Labels
C-refactor Category: Refactor E-medium Effort: Requires a fair amount of work P-low Priority: Low

Comments

@kos-for-juspay
Copy link
Contributor

https://github.com/juspay/orca/blob/01cafe753bc4ea0cf33ec604dcbe0017abfebcad/crates/common_utils/src/pii.rs#L68

We should assume constructed value object is already validated. Email validation is quite a heavy operation. Doing it on each formatting is quite a subtle performance penalty, while being... unnecessary?
Another problem, that validation here is violation of the "Separation of concerns" design principle. Formatting is not a validation in any way. https://en.wikipedia.org/wiki/Separation_of_concerns Consider to provide a newtype for email strings. https://rust-unofficial.github.io/patterns/patterns/behavioural/newtype.html

This way you do the validation only once, when creating a value of the type, and then you may fearlessly reuse it as the type system protects you. https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate
Thus, as the result, you will be able to remove any validation code from the formatting, as the compiler will guarantee that you would have valid values here. The same is true for other formatting strategies in this module too, as they're effectively validators too.

https://github.com/juspay/orca/blob/56d153d8f7c2c75391799cf14280c448df97842f/crates/router/src/connector/adyen/transformers.rs#L309

CVC shouldn't be saved in our db Will need to implement tokenization that allows us to make payments without cvv
And this could be easily prevented if it was represented as a newtype for value object which doesn't implement ToSql.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-refactor Category: Refactor E-medium Effort: Requires a fair amount of work P-low Priority: Low
Projects
None yet
Development

No branches or pull requests

1 participant