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.

Sign up for GitHub

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

Jump to bottom

fix_: normalize keycard password for pairing #6340

Merged
merged 1 commit into from
Feb 18, 2025
Merged

fix_: normalize keycard password for pairing #6340

merged 1 commit into from
Feb 18, 2025

Conversation

flexsurfer
Copy link
Member

@flexsurfer flexsurfer self-assigned this Feb 13, 2025
@flexsurfer flexsurfer force-pushed the fix_--normalize-keycard-password-for-pairing branch from 4156cbc to 0dbc513 Compare February 13, 2025 12:36
@status-im status-im deleted a comment from github-actions bot Feb 13, 2025
@status-im status-im deleted a comment from status-im-auto Feb 13, 2025
@status-im-auto
Copy link
Member

✔️ status-go/prs/macos/aarch64/main/PR-6340#1 🔹 ~4 min 54 sec 🔹 6454936 🔹 📦 macos package

@status-im-auto
Copy link
Member

✔️ status-go/prs/ios/PR-6340#1 🔹 ~5 min 14 sec 🔹 6454936 🔹 📦 ios package

@status-im-auto
Copy link
Member

✔️ status-go/prs/windows/x86_64/main/PR-6340#1 🔹 ~5 min 35 sec 🔹 6454936 🔹 📦 windows package

@status-im-auto
Copy link
Member

✔️ status-go/prs/android/PR-6340#1 🔹 ~6 min 24 sec 🔹 6454936 🔹 📦 android package

@status-im-auto
Copy link
Member

✔️ status-go/prs/linux/x86_64/main/PR-6340#1 🔹 ~6 min 29 sec 🔹 6454936 🔹 📦 linux package

@status-im-auto
Copy link
Member

✔️ status-go/prs/macos/x86_64/main/PR-6340#1 🔹 ~7 min 25 sec 🔹 6454936 🔹 📦 macos package

@status-im-auto
Copy link
Member

✔️ status-go/prs/windows/x86_64/main/PR-6340#2 🔹 ~3 min 43 sec 🔹 0dbc513 🔹 📦 windows package

@status-im-auto
Copy link
Member

✔️ status-go/prs/macos/aarch64/main/PR-6340#2 🔹 ~5 min 29 sec 🔹 0dbc513 🔹 📦 macos package

@status-im-auto
Copy link
Member

✔️ status-go/prs/ios/PR-6340#2 🔹 ~5 min 48 sec 🔹 0dbc513 🔹 📦 ios package

@flexsurfer flexsurfer mentioned this pull request Feb 13, 2025
Merged
@status-im-auto
Copy link
Member

✔️ status-go/prs/macos/x86_64/main/PR-6340#2 🔹 ~5 min 16 sec 🔹 0dbc513 🔹 📦 macos package

@status-im-auto
Copy link
Member

✔️ status-go/prs/android/PR-6340#2 🔹 ~6 min 57 sec 🔹 0dbc513 🔹 📦 android package

@status-im-auto
Copy link
Member

✔️ status-go/prs/linux/x86_64/main/PR-6340#2 🔹 ~9 min 17 sec 🔹 0dbc513 🔹 📦 linux package

Samyoul
Samyoul approved these changes Feb 13, 2025
View reviewed changes
Copy link
Member

@Samyoul Samyoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What an outrageously straight forward fix for such an involved investigation. Congratulations on this fix.

protocol/messenger_handler.go Outdated
@@ -3827,7 +3827,7 @@ func (m *Messenger) handleSyncKeypairInternal(state *ReceivedMessageState, messa
if m.walletAPI != nil {
err := m.walletAPI.SetPairingsJSONFileContent(message.KeycardPairings)
if err != nil {
return err
m.logger.Warn("handleSyncKeypairInternal unable to set pairing.json file", zap.Error(err))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably this is a non-critical error, we don't need to return it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah im not sure about this one, @saledjenic ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go with return if an error occurs.
handleSyncKeypairInternal is used in 2 scenarios:

  • while local pairing (where we use fromLocalPairing = true)
  • while applying the sync message from the paired device (where we use fromLocalPairing = false)

But an interesting thing now is that I don't see part of the code that is calling func (m *Messenger) HandleSyncKeypair(state *ReceivedMessageState, message *protobuf.SyncKeypair, statusMessage *v1protocol.StatusMessage) error function. Seems it got removed at some point. So we should remove that function if we don't need this handling (what I am not sure about).

@flexsurfer flexsurfer force-pushed the fix_--normalize-keycard-password-for-pairing branch from 0dbc513 to 063f133 Compare February 13, 2025 12:48
@status-im-auto
Copy link
Member

✔️ status-go/prs/windows/x86_64/main/PR-6340#3 🔹 ~3 min 42 sec 🔹 063f133 🔹 📦 windows package

@status-im-auto
Copy link
Member

✔️ status-go/prs/macos/aarch64/main/PR-6340#3 🔹 ~4 min 17 sec 🔹 063f133 🔹 📦 macos package

@status-im-auto
Copy link
Member

✔️ status-go/prs/ios/PR-6340#3 🔹 ~4 min 45 sec 🔹 063f133 🔹 📦 ios package

@status-im-auto
Copy link
Member

✔️ status-go/prs/linux/x86_64/main/PR-6340#3 🔹 ~5 min 46 sec 🔹 063f133 🔹 📦 linux package

@status-im-auto
Copy link
Member

✔️ status-go/prs/macos/x86_64/main/PR-6340#3 🔹 ~5 min 55 sec 🔹 063f133 🔹 📦 macos package

@status-im-auto
Copy link
Member

✔️ status-go/prs/android/PR-6340#3 🔹 ~6 min 44 sec 🔹 063f133 🔹 📦 android package

@status-im-auto
Copy link
Member

✔️ status-go/prs/tests-rpc/PR-6340#2 🔹 ~12 min 🔹 0dbc513 🔹 📦 tests-rpc package

@codecov Codecov
Copy link

codecov bot commented Feb 13, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.67%. Comparing base (b2cd7bd) to head (ae62568).
Report is 2 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #6340      +/-   ##
===========================================
- Coverage    59.67%   59.67%   -0.01%     
===========================================
  Files          865      865              
  Lines       112981   112987       +6     
===========================================
+ Hits         67418    67420       +2     
- Misses       37727    37734       +7     
+ Partials      7836     7833       -3
Flag Coverage Δ
functional 0.45% <0.00%> (-0.01%) ⬇️
unit 59.66% <100.00%> (-0.01%) ⬇️

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

Files with missing lines Coverage Δ
server/pairing/payload_management.go 87.36% <100.00%> (+0.85%) ⬆️

... and 27 files with indirect coverage changes

@status-im-auto
Copy link
Member

✔️ status-go/prs/macos/x86_64/main/PR-6340#9 🔹 ~6 min 6 sec 🔹 1d99b40 🔹 📦 macos package

@flexsurfer flexsurfer force-pushed the fix_--normalize-keycard-password-for-pairing branch from 1d99b40 to 5d8cfc6 Compare February 18, 2025 08:41
@status-im-auto
Copy link
Member

✔️ status-go/prs/ios/PR-6340#10 🔹 ~3 min 8 sec 🔹 5d8cfc6 🔹 📦 ios package

@status-im-auto
Copy link
Member

✔️ status-go/prs/android/PR-6340#10 🔹 ~3 min 19 sec 🔹 5d8cfc6 🔹 📦 android package

@status-im-auto
Copy link
Member

✔️ status-go/prs/windows/x86_64/main/PR-6340#11 🔹 ~4 min 2 sec 🔹 5d8cfc6 🔹 📦 windows package

@status-im-auto
Copy link
Member

✔️ status-go/prs/macos/aarch64/main/PR-6340#11 🔹 ~4 min 27 sec 🔹 5d8cfc6 🔹 📦 macos package

@status-im-auto
Copy link
Member

✔️ status-go/prs/linux/x86_64/main/PR-6340#10 🔹 ~6 min 9 sec 🔹 5d8cfc6 🔹 📦 linux package

@status-im-auto
Copy link
Member

✔️ status-go/prs/macos/x86_64/main/PR-6340#10 🔹 ~6 min 15 sec 🔹 5d8cfc6 🔹 📦 macos package

@status-im-auto
Copy link
Member

✔️ status-go/prs/tests-rpc/PR-6340#14 🔹 ~12 min 🔹 5d8cfc6 🔹 📦 tests-rpc package

@flexsurfer flexsurfer force-pushed the fix_--normalize-keycard-password-for-pairing branch from 5d8cfc6 to dca0890 Compare February 18, 2025 09:06
@status-im-auto
Copy link
Member

✔️ status-go/prs/ios/PR-6340#11 🔹 ~3 min 9 sec 🔹 dca0890 🔹 📦 ios package

@status-im-auto
Copy link
Member

✔️ status-go/prs/android/PR-6340#11 🔹 ~3 min 11 sec 🔹 dca0890 🔹 📦 android package

@status-im-auto
Copy link
Member

✔️ status-go/prs/windows/x86_64/main/PR-6340#12 🔹 ~3 min 41 sec 🔹 dca0890 🔹 📦 windows package

@status-im-auto
Copy link
Member

✔️ status-go/prs/macos/aarch64/main/PR-6340#12 🔹 ~4 min 27 sec 🔹 dca0890 🔹 📦 macos package

@status-im-auto
Copy link
Member

✔️ status-go/prs/linux/x86_64/main/PR-6340#11 🔹 ~5 min 12 sec 🔹 dca0890 🔹 📦 linux package

@status-im-auto
Copy link
Member

✔️ status-go/prs/macos/x86_64/main/PR-6340#11 🔹 ~5 min 59 sec 🔹 dca0890 🔹 📦 macos package

@status-im-auto
Copy link
Member

✔️ status-go/prs/tests/PR-6340#15 🔹 ~31 min 🔹 5d8cfc6 🔹 📦 tests package

@status-im-auto
Copy link
Member

✔️ status-go/prs/tests-rpc/PR-6340#15 🔹 ~12 min 🔹 dca0890 🔹 📦 tests-rpc package

@flexsurfer flexsurfer force-pushed the fix_--normalize-keycard-password-for-pairing branch from dca0890 to ae62568 Compare February 18, 2025 10:04
@status-im-auto
Copy link
Member

✔️ status-go/prs/android/PR-6340#12 🔹 ~3 min 14 sec 🔹 ae62568 🔹 📦 android package

@status-im-auto
Copy link
Member

✔️ status-go/prs/ios/PR-6340#12 🔹 ~3 min 20 sec 🔹 ae62568 🔹 📦 ios package

@status-im-auto
Copy link
Member

✔️ status-go/prs/windows/x86_64/main/PR-6340#13 🔹 ~3 min 34 sec 🔹 ae62568 🔹 📦 windows package

@status-im-auto
Copy link
Member

✔️ status-go/prs/macos/aarch64/main/PR-6340#13 🔹 ~4 min 24 sec 🔹 ae62568 🔹 📦 macos package

@status-im-auto
Copy link
Member

✔️ status-go/prs/linux/x86_64/main/PR-6340#12 🔹 ~5 min 11 sec 🔹 ae62568 🔹 📦 linux package

@status-im-auto
Copy link
Member

✔️ status-go/prs/macos/x86_64/main/PR-6340#12 🔹 ~5 min 58 sec 🔹 ae62568 🔹 📦 macos package

@status-im-auto
Copy link
Member

✔️ status-go/prs/tests-rpc/PR-6340#17 🔹 ~11 min 🔹 ae62568 🔹 📦 tests-rpc package

@status-im-auto
Copy link
Member

✔️ status-go/prs/tests/PR-6340#17 🔹 ~31 min 🔹 ae62568 🔹 📦 tests package

@flexsurfer flexsurfer merged commit 1bfb0ce into develop Feb 18, 2025
19 checks passed
@flexsurfer flexsurfer deleted the fix_--normalize-keycard-password-for-pairing branch February 18, 2025 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Samyoul Samyoul Samyoul approved these changes

@igor-sirotin igor-sirotin igor-sirotin approved these changes

@saledjenic saledjenic saledjenic approved these changes

Assignees

@flexsurfer flexsurfer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@flexsurfer @status-im-auto @Samyoul @igor-sirotin @saledjenic