Skip to content

Conversation

@rg911
Copy link
Contributor

@rg911 rg911 commented Jul 16, 2019

Bug fixed in newly introduced signTransactionGivenSignatures(). It is noticed that the signed transaction's signature mismatched with standard cosign methods after applied generationHash.

this PR:

  • Fixed generationHash should be passed in as byteArray rather than binary in signTransactionPayload
  • Added e2d test for signTransactionGivenSignatures()
  • Updated unit tests
  • Added signSchema to signTransactionPayload
  • Fixed typos

**Process flow changes

It was initially proposed to generate 'plain` (empty signature & signer) payload by the initiator and passed to cosigners. However, the first half of the initiator's signature is required to validate co-signatures. So it is changed that:

  1. Alice creates the aggregated transaction and sign it, Then signed payload send to Bob & Carol
  2. Bob and Carol cosigns the transaction using signTransactionPayload method and sends it back to Alice
  3. Alice collects the cosignatures, recreate, sign it using signTransactionGivenSignatures, and announces the transaction

Issue : #204

rg911 added 8 commits July 8, 2019 22:45
- Added `restrictable` in MosaicProperties
- Added MosaicGlobalRestriction and MosaicAddressRestriction transactions
- Added unit tests and e2e tests for the 2 new txs
- Added serialization handlers
- Fixed couple of serialization bugs with MosaicId
- Fixed signTransactionGivenSignatures bug
- Added e2e test for signTransactionGivenSignatures
- Improved unit test for signTransactionGivenSignatures
- Fixed typo
@coveralls
Copy link

coveralls commented Jul 16, 2019

Pull Request Test Coverage Report for Build 588

  • 457 of 636 (71.86%) changed or added relevant lines in 21 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 60.759%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/model/account/Account.ts 0 1 0.0%
src/infrastructure/builders/MosaicAddressRestrictionTransaction.ts 56 58 96.55%
src/infrastructure/builders/MosaicGlobalRestrictionTransaction.ts 61 63 96.83%
src/model/transaction/MosaicAddressRestrictionTransaction.ts 14 22 63.64%
src/model/transaction/MosaicGlobalRestrictionTransaction.ts 16 26 61.54%
src/infrastructure/buffers/MosaicAddressRestrictionTransactionBuffer.ts 120 196 61.22%
src/infrastructure/buffers/MosaicGlobalRestrictionTransactionBuffer.ts 126 206 61.17%
Totals Coverage Status
Change from base Build 568: -0.03%
Covered Lines: 6360
Relevant Lines: 9034

💛 - Coveralls

expect(signedTransaction.payload.indexOf(accountCarol.publicKey) > -1).to.be.true;

// To make sure that the new cosign method returns the same payload & hash as standard cosigning
const standardCosignedTransaction = aggregateTransaction
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Grégory Saive and others added 9 commits July 20, 2019 14:00
Fixed compatibility issue on new rxjs version
…compatibility

Revert "Fixed compatibility issue on new rxjs version"
- Fixed signTransactionGivenSignatures bug
- Added e2e test for signTransactionGivenSignatures
- Improved unit test for signTransactionGivenSignatures
- Fixed typo
…:rg911/nem2-sdk-typescript-javascript into task/g204_signTransactionGivenSignatures
@rg911 rg911 merged commit 716c4db into symbol:master Jul 25, 2019
@rg911 rg911 deleted the task/g204_signTransactionGivenSignatures branch October 8, 2019 20:18
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.

3 participants