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

Added #112 sign with cosignatories #131

Merged
merged 202 commits into from Jul 3, 2019

Conversation

Projects
None yet
4 participants
@rg911
Copy link
Member

commented Apr 25, 2019

Issue: #112

Added a few methods to cope with off chain aggregated complete transaction co signing.

Please refer to this scenario currently been discussed.

Problem found from discussion:

  1. Alice needs to create - sign - recreate - sign again
  2. CosignatureTransaction cannot be created before aggregated transaction's been announced.
  3. signTransactionWithCosignatories(initiatorAccount: Account, cosignatories: Account[]) requires both initiator and the cosigners's account details, which makes it impossible to sign off chain.

Solution proposed in this PR:

  1. Alice creates the aggregated tx and serialize it, and generate the hash. Then send to Bob & Carol

    • Use the newly exposed transaction.serialize() methods to create transaction payload (without an initial signing). This method will create binary payload with signer and signatures showing as 0000000.....0000
    • transaction payload can then be send to Bob & Carol off the chain.
  2. Bob & Carol validate the transaction and cosigns the tx and sends it back to Alice

    • Created a new method CosignatureTransaction.signTransactionPayload(account, transactionPayload) which can sign the transaction with its payload.
    • It will return object CosignatureSignedTransaction with signatures Alice expected.
    const signedTxBob = CosignatureTransaction.signTransactionPayload(accountBob, txPayloadHex);
    const signedTxCarol = CosignatureTransaction.signTransactionPayload(accountCarol, txPayloadHex);
    
  3. Alice collects the cosignatures, recreate, sign, and announces the transaction

    • Alice will collect those signatures.
    const cosignatureSignedTransactions = [
          new CosignatureSignedTransaction(signedTxBob.parentHash, signedTxBob.signature, signedTxBob.signer),
          new CosignatureSignedTransaction(signedTxCarol.parentHash, signedTxCarol.signature, signedTxCarol.signer),
      ]; 
    
    • Alice then recreate the ACT from payload
    const recreatedTx = TransactionMapping.createFromPayload(aggregateTransactionPayload) as AggregateTransaction;
    
    • To sign the transaction, Alice now use the new method which does not require both initiator Account and cosigners accounts.
    aggregatedTx.signTransactionGivenSignatures(accountAlice, cosignatureSignedTransactions);
    

@rg911 rg911 requested review from evias and dgarcia360 Apr 25, 2019

@jontey

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

Passing just the hash around is dangerous, as Bob and Carol will not know what they are signing.

I would suggest that CosignatureTransaction.signTransactionHashWith receive a transaction payload rather than just the hash to force developers to pass the entire transaction rather than just the hash (even though the function doesn't do any checks).

The reason behind this is that if the function accepts a hash, most developers will just pass around the hash, making applications that are vulnerable (signing compromised transactions). By forcing the transaction payload, it will notify the developers to think, Hey, maybe I need to get the other side to check if the transaction I'm signing is correct; or make it easier for developers to implement the check (on Bob and Carol's side).

@rg911

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

Passing just the hash around is dangerous, as Bob and Carol will not know what they are signing.

I would suggest that CosignatureTransaction.signTransactionHashWith receive a transaction payload rather than just the hash to force developers to pass the entire transaction rather than just the hash (even though the function doesn't do any checks).

The reason behind this is that if the function accepts a hash, most developers will just pass around the hash, making applications that are vulnerable (signing compromised transactions). By forcing the transaction payload, it will notify the developers to think, Hey, maybe I need to get the other side to check if the transaction I'm signing is correct; or make it easier for developers to implement the check (on Bob and Carol's side).

Fair point, cheers @jontey 👍

@evias

evias approved these changes May 1, 2019

@evias

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

tests failing due to needed library update

@evias

evias approved these changes May 2, 2019

evias and others added some commits May 2, 2019

Merge branch 'task/g112_sign_with_cosignatories' of github.com:rg911/…
…nem2-sdk-typescript-javascript into task/g112_sign_with_cosignatories
@coveralls

This comment has been minimized.

Copy link

commented Jun 17, 2019

Pull Request Test Coverage Report for Build 561

  • 8 of 8 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 60.791%

Totals Coverage Status
Change from base Build 558: 0.1%
Covered Lines: 5916
Relevant Lines: 8413

💛 - Coveralls

@evias evias merged commit dff0235 into nemtech:master Jul 3, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rg911 rg911 deleted the rg911:task/g112_sign_with_cosignatories branch Jul 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.