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

python rola implementation #14

Open
wants to merge 49 commits into
base: main
Choose a base branch
from
Open

python rola implementation #14

wants to merge 49 commits into from

Conversation

balda-rdx
Copy link

No description provided.

python/rola/config/network.py Outdated Show resolved Hide resolved
python/tests/test_signed_challenge.py Outdated Show resolved Hide resolved
python/tests/test_ret.py Show resolved Hide resolved
@balda-rdx balda-rdx changed the title EC-3: python rola python rola implementation Nov 29, 2023
Copy link
Member

@0xOmarA 0xOmarA left a comment

Choose a reason for hiding this comment

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

Looks good overall but I think that there are some areas where changes are needed. Some of the most notable things that I think we should change are:

  • Most functions either accept or return hex string and then immediately convert them into bytes to use them. I would recommend to stop using hex strings completely for function signatures and only use bytes to avoid redundant conversions between hex string and bytes.
  • I would recommend we rely more on enums especially in areas where we know that the user can't provide us with arbitrary string or arbitrary input. An example of this is the challenge.proof.curve and the challenge.type. Also, depending on whether you think the gateway suggestion I provided is the way to go then network might also be an enum.
  • Lets format this code base with a consistent formatter and check for it in CI. I recommend black because it's super simple and not-configurable.
  • I believe that more unit tests are needed.

examples/typescript-rola-as-a-service/.sst/stage Outdated Show resolved Hide resolved
python/LICENSE Outdated Show resolved Hide resolved
python/rola/config/network.py Outdated Show resolved Hide resolved
python/Pipfile.lock Show resolved Hide resolved
python/setup.py Outdated Show resolved Hide resolved
python/rola/utils/helpers.py Outdated Show resolved Hide resolved
python/rola/utils/gateway.py Outdated Show resolved Hide resolved
python/rola/core.py Outdated Show resolved Hide resolved
python/rola/core.py Outdated Show resolved Hide resolved
python/rola/core.py Outdated Show resolved Hide resolved
@balda-rdx
Copy link
Author

Looks good overall but I think that there are some areas where changes are needed. Some of the most notable things that I think we should change are:

  • Most functions either accept or return hex string and then immediately convert them into bytes to use them. I would recommend to stop using hex strings completely for function signatures and only use bytes to avoid redundant conversions between hex string and bytes.
  • I would recommend we rely more on enums especially in areas where we know that the user can't provide us with arbitrary string or arbitrary input. An example of this is the challenge.proof.curve and the challenge.type. Also, depending on whether you think the gateway suggestion I provided is the way to go then network might also be an enum.
  • Lets format this code base with a consistent formatter and check for it in CI. I recommend black because it's super simple and not-configurable.
  • I believe that more unit tests are needed.

thanks for the feedback @0xOmarA . yes I've already added black, it is really nice and easy indeed.

Good call about the use of hex and bytes. will make the suggested change

I'm indeed adding more unit tests 👍

Copy link
Member

@0xOmarA 0xOmarA left a comment

Choose a reason for hiding this comment

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

Left a few comments on areas that I think we should change before we merge.

@0xOmarA
Copy link
Member

0xOmarA commented Dec 15, 2023

Hey @balda-rdx, curious if this is ready for re review?

@balda-rdx
Copy link
Author

Hey @balda-rdx, curious if this is ready for re review?

@0xOmarA yes yes 👍

@0xOmarA
Copy link
Member

0xOmarA commented Jan 5, 2024

Fantastic, thanks @balda-rdx, will review in coming days.

python/rola/utils/ret.py Outdated Show resolved Hide resolved
Copy link
Member

@0xOmarA 0xOmarA left a comment

Choose a reason for hiding this comment

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

There's one comment that I left a ping on that we must address before we merge but aside from that I think this is good to go.

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.

4 participants