-
Notifications
You must be signed in to change notification settings - Fork 284
New Extension: SK17 #2206
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
base: master
Are you sure you want to change the base?
New Extension: SK17 #2206
Conversation
!format |
!format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that the file-loading blocks should be included, since the Files extension renders per-extension file handling largely useless. See #1492 for a similar case involving useless blocks that I proposed.
!format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems better than before (no file handling in this version), but I'm concerned about your usage of low-level cryptography APIs. It's very easy to make a slip-up, and it will be a pain to migrate encrypted data if you realize that your old method was flawed.
As I am apparently responsible enough for someone to think I should do code reviews, it is my duty to provide an honest review. JavaScript is one of my least-favorite practical languages because the standard library has so many missing features that stuff like this crops up easily. I don't mean to burst your bubble, but you're gonna have a fun time finding an MIT-compatible high-level JavaScript library for encryption that works in a single blob of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it and it seems to work. Before I give my approval, I'd like to clarify a few things...
First, you shouldn't use the master branch; try to leave it blank and have different branches for each pull request/extension. This keeps things organized for future contributions. If you're confused and need more information just ask.
Second, see the few short notes below.
images/SkyBuilder1717/SK17.svg
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Images can't contain text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? What about Simple 3D or Turbohook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? What about Simple 3D or Turbohook?
text in images can't be translated and it's not good for typography. I think Turbohook was added fairly before that soft requirement and I think there was an argument for keeping it for Simple3D (the banner was rendered with the extension)...
Co-authored-by: Brackets-Coder <[email protected]>
not opposed to the idea of having blocks that wrap crypto primitives but the description "Useful for saving games and loading private data in compiled games." is probably very misleading since the keys are in the project, so anyone can just unpackage it and get the key out. claims about the security of a system should have a strong basis |
SK17 is an encrypting extension, for example, saving games and loading private data in compiled games!
