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

Merge security audited changes #2

Merged
merged 33 commits into from
Jan 3, 2024

Conversation

coderofstuff
Copy link

@coderofstuff coderofstuff commented Dec 4, 2023

Checklist

  • App update process has been followed
  • Target branch is develop
  • Application version has been bumped

@xchapron-ledger
Copy link

@coderofstuff Can you rebase your PR? Also have you followed the PR template question?
I don't see a version bump, have you followed the app update process?

@coderofstuff
Copy link
Author

@coderofstuff Can you rebase your PR? Also have you followed the PR template question? I don't see a version bump, have you followed the app update process?

Do I need a version bump even though this hasn't been publicly released? That is, a version bump is required for every PR?

Where is the app update process? I put this PR up because I was asked to put a PR up to merge in the security audited commit. If it's fine for me to merge your develop into my branch then sure I'll do that.

cc. @vforgeoux-ledger

@xchapron-ledger
Copy link

Do I need a version bump even though this hasn't been publicly released? That is, a version bump is required for every PR?

The version bump is necessary even if the app didn't reach the public, indeed a version 1.0.0 has been deployed for internal test. It can be seen in the tags here: https://github.com/LedgerHQ/app-kaspa/tags
Though a PR bumping the version has been merged in develop meanwhile...
But bumping the version is simple, and helps easily identicate what is embedded in a specific version.

Where is the app update process? I put this PR up because I was asked to put a PR up to merge in the security audited commit.

I'm not sure, best is to discuss with Victor F on Discord.

If it's fine for me to merge your develop into my branch then sure I'll do that.

I would prefer a rebase because I prefer linear git history, but I understand that this will impact your repo history, so as you prefer.

Merge in Ledger's changes in their "develop"
@coderofstuff
Copy link
Author

@xchapron-ledger I've merged develop into my main. I generally prefer rebases too but I can't afford to do that on the main branch of my repo. I'll do that for future updates when I create my own develop branch.

I see there is indeed a version bump here making it be 1.0.1. Do you still need me to make it 1.0.2?

I'll check in with Victor for the app update process when he gets back.

@xchapron-ledger
Copy link

I see there is indeed a version bump here making it be 1.0.1. Do you still need me to make it 1.0.2?

Yep that would be preferable.

@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2023

Codecov Report

Attention: 52 lines in your changes are missing coverage. Please review.

Comparison is base (648c585) 91.71% compared to head (ae2bff4) 80.56%.

Files Patch % Lines
src/sighash.c 60.39% 40 Missing ⚠️
src/transaction/utils.c 58.33% 5 Missing ⚠️
src/personal_message.c 75.00% 4 Missing ⚠️
src/transaction/deserialize.c 78.57% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           develop       #2       +/-   ##
============================================
- Coverage    91.71%   80.56%   -11.16%     
============================================
  Files            7        7               
  Lines          326      391       +65     
============================================
+ Hits           299      315       +16     
- Misses          27       76       +49     
Flag Coverage Δ
unittests 80.56% <65.56%> (-11.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@agrojean-ledger agrojean-ledger merged commit 1d19f4f into LedgerHQ:develop Jan 3, 2024
39 checks passed
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