-
Notifications
You must be signed in to change notification settings - Fork 55
Bitcoin-only firmware integration #658
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
Conversation
cy-utkarshM
commented
Jun 16, 2025
- Added base features
- Refactored firmware layout
- Backed up intermediate working states
resolve conflicts and fix ci |
35f10d2
to
853a3c4
Compare
853a3c4
to
8004cf4
Compare
apps/btc_family/btc_pub_key.c
Outdated
memcpy(data.params, | ||
request->initiate.wallet_id, | ||
sizeof(request->initiate.wallet_id)); | ||
#ifndef BTC_ONLY_BUILD |
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.
Move the above defined variables(for exchange app function call) also inside the if block
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.
done
apps/btc_family/btc_pub_key.c
Outdated
memzero(seed, sizeof(seed)); | ||
|
||
if (sign_address) { | ||
#ifndef BTC_ONLY_BUILD |
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.
Keep the whole if (sign_address) block inside the #ifndef BTC_ONLY_BUILD block. Also, keep the sign_address variable declaration statement in the #ifndef BTC_ONLY_BUILD
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.
done
apps/btc_family/btc_txn.c
Outdated
} | ||
|
||
if (use_signature_verification) { | ||
#ifndef BTC_ONLY_BUILD |
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.
Move all code related to exchange app inside the #ifndef BTC_ONLY_BUILD block. Be it variable(global or local) declarations or if conditions or function calls.
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.
Removed static bool use_signature_verification = false; from the global/static variables section.
Added bool use_signature_verification = false; inside the #ifndef BTC_ONLY_BUILD block within validate_request_data. This makes use_signature_verification a local variable to validate_request_data when BTC_ONLY_BUILD is not defined.
Added bool use_signature_verification = false; inside the #ifndef BTC_ONLY_BUILD block within get_user_verification. This makes use_signature_verification a local variable to get_user_verification when BTC_ONLY_BUILD is not defined. I also kept your comment explaining the current state of this variable's usage.
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 is static bool use_signature_verification = false; removed from the global/static variables section and added as local variable? This breaks exchange flow.
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've pushed a fix that corrects this. use_signature_verification is now a static variable again at the file level to preserve its state across function calls, but the entire declaration is properly wrapped within the #ifndef BTC_ONLY_BUILD block.
// free(meta_data_arr[i].data_struct.coin_data); | ||
// } | ||
// } | ||
// } |
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 is this code commented?
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.
this code was never used, after talking to Pulkit I removed it.
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.
Yes, but we can also just guard it.
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.
Okay then I will guard it then!
common/core/core_flow_init.c
Outdated
registry_add_app(get_ltc_app_desc()); | ||
registry_add_app(get_doge_app_desc()); | ||
registry_add_app(get_dash_app_desc()); | ||
registry_add_app(get_inheritance_app_desc()); |
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.
inheritance app is added twice
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.
done
a505414
to
a2fe260
Compare
d736a97
to
6683767
Compare
47379d7
to
ad7911c
Compare
ad7911c
to
54bf3b4
Compare
.github/workflows/build.yml
Outdated
content_type=$(file -b --mime-type Main-Release-outputs/Cypherock-Main.bin) | ||
curl -X POST -H "Content-Type: ${content_type}" -H "Accept: application/vnd.github+json" -H "Authorization: Bearer ${auth_token}" -H "X-GitHub-Api-Version: 2022-11-28" ${upload_url}?name=Cypherock-Main.bin --data-binary @Main-Release-outputs/Cypherock-Main.bin | ||
curl -X POST -H "Content-Type: ${content_type}" -H "Accept: application/vnd.github+json" -H "Authorization: Bearer ${auth_token}" -H "X-GitHub-Api-Version: 2022-11-28" ${upload_url}?name=version.txt --data-binary @version.txt | ||
curl -X POST -H "Content-Type: ${content_type}" -H "Accept: application/vnd.github+json" -H "Authorization: Bearer ${auth_token}" -H "X-GitHub-Api-Version: 2022-11-28" ${upload_url}?name=version.txt --data-binary @version.txt |
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.
Upload btc only firmware to github also. Also generate its hash. Take reference from odix pr
CMakeLists.txt
Outdated
OPTION(DEV_SWITCH "Additional features/logs to aid developers" OFF) | ||
OPTION(UNIT_TESTS_SWITCH "Compile build for main firmware or unit tests" OFF) | ||
OPTION(BTC_ONLY "Build firmware for Bitcoin only" OFF) | ||
|
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.
In case of btc only, append -btc to project name. Take reference from odix pr
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.
done!
"${PROTO_COMMON_SRC}/manager" | ||
"${PROTO_COMMON_SRC}/btc" | ||
"${PROTO_COMMON_SRC}/inheritance" | ||
) |
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.
Top level common proto files(like core.proto, error.proto) are missing in btc only
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.
Can you explain the need to change the variable name EXECUTABLE
to PROJECT
The PROJECT_NAME variable already comes from the top-level CMakeLists.txt, and it is guaranteed to be the correct project identifier (e.g., Cypherock-Main-btc). |
This reverts commit 4b242d2.
This reverts commit b92ed97.
bf34ef8
to
e171b0c
Compare