Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Fix missing multisig signatures #9095

Merged
merged 4 commits into from
Oct 25, 2023

Conversation

bobanm
Copy link
Contributor

@bobanm bobanm commented Oct 16, 2023

What was the problem?

This PR resolves #9094

How was it solved?

Changed transaction:sign command to no longer hardcode private key derivation to legacy method. Now it uses deriveKeypair() function which will use either legacy or key path derivation, depending on the provided --key-derivation-path flags.

How was it tested?

When signing a multisig transactions offline, every signing from a different multisig signer now adds an additional signature to transaction's signatures array.

@bobanm bobanm requested review from shuse2 and Incede October 16, 2023 23:27
@bobanm bobanm self-assigned this Oct 16, 2023
@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Merging #9095 (0097fc5) into release/6.0.0 (c9c1ecc) will increase coverage by 0.04%.
Report is 14 commits behind head on release/6.0.0.
The diff coverage is 96.81%.

Impacted file tree graph

@@                Coverage Diff                @@
##           release/6.0.0    #9095      +/-   ##
=================================================
+ Coverage          83.47%   83.51%   +0.04%     
=================================================
  Files                604      606       +2     
  Lines              22803    22828      +25     
  Branches            3364     3365       +1     
=================================================
+ Hits               19035    19065      +30     
+ Misses              3768     3763       -5     
Files Coverage Δ
...der/src/bootstrapping/commands/transaction/sign.ts 93.05% <100.00%> (+0.95%) ⬆️
commander/test/helpers/account.ts 31.03% <100.00%> (+2.46%) ⬆️
elements/lisk-chain/src/block_header.ts 73.13% <100.00%> (ø)
...ain-connector-plugin/src/certificate_generation.ts 97.95% <100.00%> (+0.04%) ⬆️
...ork-monitor-plugin/src/controllers/transactions.ts 22.22% <ø> (ø)
framework/src/engine/bft/method.ts 88.97% <100.00%> (+0.08%) ⬆️
...ne/consensus/certificate_generation/commit_list.ts 100.00% <100.00%> (ø)
framework/src/genesis_block.ts 86.95% <ø> (ø)
...nteroperability/base_cross_chain_update_command.ts 97.71% <100.00%> (+0.06%) ⬆️
...rability/base_interoperability_internal_methods.ts 94.13% <100.00%> (+1.20%) ⬆️
... and 20 more

... and 3 files with indirect coverage changes

Copy link
Collaborator

@shuse2 shuse2 left a comment

Choose a reason for hiding this comment

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

LGTM, as we discussed, it would be good to add a test for the default key derivation as well for checking integration

@bobanm bobanm requested a review from shuse2 October 19, 2023 00:08
@shuse2 shuse2 merged commit 0056f19 into release/6.0.0 Oct 25, 2023
10 checks passed
@shuse2 shuse2 deleted the 9094-fix-missing-multisig-signatures branch October 25, 2023 07:27
@TalhaMaliktz TalhaMaliktz restored the 9094-fix-missing-multisig-signatures branch October 25, 2023 10:53
@TalhaMaliktz TalhaMaliktz deleted the 9094-fix-missing-multisig-signatures branch October 25, 2023 10:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants