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

Fix keycard L1-L1 transaction failing #21589

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

clauxx
Copy link
Member

@clauxx clauxx commented Nov 6, 2024

fixes #21533

Summary

Seems like the issue is in the v component of the signature and the values that are passed. Looking at this status-go function, if the value is 1, it gets set to 0 instead, invalidating the signature. Seems like for a v of 1, we should just send "01" as it expects, instead of parsing it and sending "1".

So it makes sense that it would fail just sometimes, since when v is 0 it should work as expected, cause it's the default.

		rBytes, _ := hex.DecodeString(sigDetails.R)
		sBytes, _ := hex.DecodeString(sigDetails.S)
		vByte := byte(0)
		if sigDetails.V == "01" { // here is the check, but it's weird 
			vByte = 1
		}

		desc.signature = make([]byte, crypto.SignatureLength)
		copy(desc.signature[32-len(rBytes):32], rBytes)
		copy(desc.signature[64-len(rBytes):64], sBytes)
		desc.signature[64] = vByte // otherwise becomes 0 here

Testing

I checked a few times and seems to work, but don't have enough funds to test it as much

status: ready

@status-im-auto
Copy link
Member

status-im-auto commented Nov 6, 2024

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
1e7d538 #1 2024-11-06 14:33:39 ~3 min ios 📄log
✔️ 1e7d538 #1 2024-11-06 14:34:51 ~4 min tests 📄log
✔️ 1e7d538 #1 2024-11-06 14:38:33 ~8 min android-e2e 🤖apk 📲
✔️ 1e7d538 #1 2024-11-06 14:39:01 ~8 min android 🤖apk 📲

@clauxx clauxx requested a review from alwx November 6, 2024 14:34
@VolodLytvynenko VolodLytvynenko self-assigned this Nov 6, 2024
@status-im-auto
Copy link
Member

88% of end-end tests have passed

Total executed tests: 8
Failed tests: 0
Expected to fail tests: 1
Passed tests: 7
IDs of expected to fail tests: 702843 

Expected to fail tests (1)

Click to expand

Class TestCommunityMultipleDeviceMerged:

1. test_community_message_edit, id: 702843
Test is not run, e2e blocker  

[[reason: [NOTRUN] Skipped due to waku issue on staging fleet]]

Passed tests (7)

Click to expand

Class TestCommunityOneDeviceMerged:

1. test_community_copy_and_paste_message_in_chat_input, id: 702742
Device sessions

2. test_restore_multiaccount_with_waku_backup_remove_profile_switch, id: 703133
Device sessions

Class TestOneToOneChatMultipleSharedDevicesNewUi:

1. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
Device sessions

Class TestWalletOneDevice:

1. test_wallet_add_remove_regular_account, id: 727231
2. test_wallet_balance_mainnet, id: 740490

Class TestWalletMultipleDevice:

1. test_wallet_send_asset_from_drawer, id: 727230
2. test_wallet_send_eth, id: 727229

@@ -6,7 +6,7 @@
[tx-hash signature]
{tx-hash {:r (subs signature 0 64)
:s (subs signature 64 128)
:v (str (js/parseInt (subs signature 128 130) 16))}})
:v (subs signature 128 130)}})
Copy link
Member

Choose a reason for hiding this comment

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

Could we move this function to a utils file?
Also, wdyt of adding unit tests for this function? I think it can serve as documentation for a complex feature like keycard.

@alwx
Copy link
Contributor

alwx commented Nov 6, 2024

We can merge this one but in the new transaction flow I already moved it to utils namespace (and fixed as well):
https://github.com/status-im/status-mobile/pull/21541/files#diff-6379c7a5055825d77e6e5e5f78afb970e1a4bbf691173be9c91076d0fcc426f7R263

@VolodLytvynenko
Copy link
Contributor

hi @clauxx thank you for PR. No issues from my side. PR is ready to be merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: MERGE
Development

Successfully merging this pull request may close these issues.

Insufficient funds...0 balance error when signing L1-L1 transaction by keycard
6 participants