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

Phoenix: creates initial sceleton with a single err and a copy of pai… #116

Merged
merged 8 commits into from
Aug 31, 2023

Conversation

gangov
Copy link
Collaborator

@gangov gangov commented Aug 25, 2023

…r/utils.rs. Once implemented in full we need to pick it up from here and do some heavy refactoring

@gangov gangov linked an issue Aug 25, 2023 that may be closed by this pull request
@gangov gangov self-assigned this Aug 25, 2023
@gangov gangov marked this pull request as ready for review August 25, 2023 18:48
Copy link
Member

@ueco-jb ueco-jb left a comment

Choose a reason for hiding this comment

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

Don't use common structure for all the errors, it's not a good idea.
Don't leave commented code.

I'm still thinking if phoenix is a good name. Seems like it, given that it would become a core component for probably most of out contracts, but may be confusing.
Let's leave it as it is, just pointing that out.

contracts/pair/src/tests/setup.rs Show resolved Hide resolved
contracts/pair/src/utils.rs Outdated Show resolved Hide resolved
packages/phoenix/src/lib.rs Outdated Show resolved Hide resolved
packages/phoenix/src/error.rs Outdated Show resolved Hide resolved
@gangov
Copy link
Collaborator Author

gangov commented Aug 28, 2023

about the name of the new library - yeah I also thought of that, as I was constantly getting confused. We probably can go with something more verbose probably phoenix_helper, but we can give it a thought later

@gangov gangov force-pushed the 113-pair-move-utils-into-a-separate-library branch from ec33e70 to 960297b Compare August 29, 2023 16:05
Copy link
Member

@ueco-jb ueco-jb left a comment

Choose a reason for hiding this comment

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

LGTM
Good work; you can merge it and modify that test I commented out in follow up or in this one and just merge it after.

packages/phoenix/src/utils.rs Outdated Show resolved Hide resolved
@gangov gangov force-pushed the 113-pair-move-utils-into-a-separate-library branch from 960297b to e842dde Compare August 31, 2023 14:28
…r/utils.rs. Once implemented in full we need to pick it up from here and do some heavy refactoring
…and makes chnages to the pair and factory cargo.tomls
…re we don't store errors in the helper lib

Pair: adds the helper macro validate_int_parameters
Pair-stable: adds the helper macor validate_int_parameters
@gangov gangov force-pushed the 113-pair-move-utils-into-a-separate-library branch from e86b7b4 to da135b0 Compare August 31, 2023 15:11
@gangov gangov merged commit 7dc6f91 into main Aug 31, 2023
3 checks passed
@gangov gangov deleted the 113-pair-move-utils-into-a-separate-library branch August 31, 2023 15:29
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.

Pair: Move utils into a separate library
2 participants