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

feat: refactor thu-info-lib/src/util/srunCrypto #570

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chillcicada
Copy link

🎉 Thanks for taking time to contribute! 🎉

Before we move on, fill out the template so that we can better understand your contribution.

What is the purpose of this PR?

  • Fixing a bug
  • Adding a feature (not even a feature, just clean the messy code

Is there a related issue?

no

Please describe your changes.

I'm recently contributing to a similiar project. I just refer to and refactor some codes from here.

This pr is aimed to prune and clarify the code in the thu-info-lib/src/util/srunCrypto file. And i just provide the minimal requirement for the srun network. (if you want a class with encode/decode, please uncomment the rest part, or you can just remove that)

Are there any possible drawbacks or side-effects?

no, i think

How to verify the changes?

with unit tests, and i'm just refactor the code :)

as a windows user, i haven't made sense of how to test it🤧, maybe a more useful guide is needed.

but i have test the relevant changes on other project, see here. hint that the expected outputs are the same as the origin code, and the base64 decode part will always break the test file (maybe unexpected eof) so i only make test for xEncode and base64encode. All the tests / typecheck are passed.

For i only make units test for the changed part, code review is needed for further tests.

@84634E1A607A
Copy link
Contributor

Thanks for contributing~ We copied these code from Srun's javascript source. We appreciate the changes made, but preferrably we expect a base64 implementation with native base64 functionality and a custom character map. (Btw there are still some bugs in this functionallity so I may investigate maybe today.)

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.

None yet

2 participants