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

MaixPy should be a submodule #78

Closed
Giszmo opened this issue Feb 8, 2022 · 5 comments · Fixed by #102
Closed

MaixPy should be a submodule #78

Giszmo opened this issue Feb 8, 2022 · 5 comments · Fixed by #102
Milestone

Comments

@Giszmo
Copy link

Giszmo commented Feb 8, 2022

Copying code in makes code review hard as now the reviewer also has check where that copied code came from and if it was modified. #39 sounds like this issue might be resolved by removal of the code but anyway, for a Bitcoin wallet code auditability is key.

@ghost
Copy link

ghost commented Feb 9, 2022

The changes made to MaixPy are extensive enough (and specific to Krux, such as memory wiping and QR code generation) that in order to do a proper audit, you do need to review the firmware code regardless.

I think making it a submodule eventually would be worthwhile (if/when Krux is more platform-agnostic), but for now since the firmware code is Krux-specific there isn't much reason to break it out yet. I'll leave this issue open though since it makes sense to do that in the future.

(By the way, if you're planning to audit the code, you may want to either start in the tests branch or wait until that makes its way into main in the next few weeks since it includes a fair amount of code changes as well. That, or wait until I put out the first official release soon after.)

@Giszmo
Copy link
Author

Giszmo commented Feb 9, 2022

As a regular code auditor in general it's impossible to individually review every line of an extensive project, so one takes short cuts. One of those is to trust dependencies that are deemed to probably not target private keys and only look at the diff from there. Therefore it would be really helpful to have MaixPy as a submodule with its original history preserved and the necessary changes on top. For an auditor, "breaking it out" into a submodule without its historic commits is not helpful.

@ghost
Copy link

ghost commented Feb 10, 2022

Okay, I see. I can certainly try to do that to make auditing easier 👍

@ghost ghost added this to the Version 1.0.0 milestone Mar 8, 2022
@ghost ghost mentioned this issue Mar 11, 2022
@ghost ghost closed this as completed in #102 Mar 21, 2022
@ghost
Copy link

ghost commented Mar 21, 2022

@Giszmo Hey, I just wanted to give you notice that I'm planning to put out the first official release (v1.0.0) of Krux this week!

@ghost
Copy link

ghost commented Mar 31, 2022

@Giszmo Here it is!
https://github.com/selfcustody/krux/releases/tag/v22.03.0

This issue was closed.
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 a pull request may close this issue.

1 participant