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

Improve signing process for a fully signed transaction - Closes #4059 #4070

Merged
merged 4 commits into from
Jan 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/components/screens/signMultiSignTransaction/styles.css
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,8 @@

.txDetails {
max-height: calc(100vh - 80px - 290px); /* stylelint-disable-line unit-whitelist */

&.small {
max-height: calc(100vh - 110px - 290px); /* stylelint-disable-line unit-whitelist */
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const Summary = ({
nextStep({
rawTransaction: transaction,
sender: senderAccount,
signatureStatus,
});
};

Expand Down Expand Up @@ -70,7 +71,7 @@ const Summary = ({
data: transaction,
error,
}}
containerStyle={styles.txDetails}
containerStyle={`${styles.txDetails} ${showFeedback && isMember ? styles.small : ''}`}
/>
</BoxContent>
{
Expand Down
3 changes: 2 additions & 1 deletion src/components/shared/transactionSignature/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import { withRouter } from 'react-router';
import { connect } from 'react-redux';
import { compose } from 'redux';
import { multisigTransactionSigned, transactionDoubleSigned } from '@actions';
import { multisigTransactionSigned, transactionDoubleSigned, signatureSkipped } from '@actions';
import { withTranslation } from 'react-i18next';
import TransactionSignature from './transactionSignature';

Expand All @@ -14,6 +14,7 @@ const mapStateToProps = state => ({
const dispatchToProps = {
multisigTransactionSigned,
transactionDoubleSigned,
signatureSkipped,
};

export default compose(
Expand Down
14 changes: 11 additions & 3 deletions src/components/shared/transactionSignature/transactionSignature.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React, { useEffect } from 'react';
import { signatureCollectionStatus } from '@constants';
import Box from '@toolbox/box';
import Illustration from '@toolbox/illustration';
import BoxContent from '@toolbox/box/content';
Expand All @@ -9,6 +10,7 @@ import styles from './transactionSignature.css';
const TransactionSignature = ({
t, transactions, account, actionFunction, multisigTransactionSigned,
rawTransaction, nextStep, statusInfo, sender, transactionDoubleSigned,
signatureStatus, signatureSkipped,
}) => {
const deviceType = getDeviceType(account.hwInfo?.deviceModel);

Expand All @@ -19,9 +21,15 @@ const TransactionSignature = ({
* sender account is required.
*/
if (sender) {
multisigTransactionSigned({
rawTransaction, sender,
});
if (signatureStatus === signatureCollectionStatus.fullySigned
|| signatureStatus === signatureCollectionStatus.overSigned) {
// Skip the current member as the all required signature are collected
signatureSkipped({ rawTransaction });
} else {
multisigTransactionSigned({
rawTransaction, sender,
});
}
} else {
/**
* The action function must be wrapped in dispatch
Expand Down
1 change: 1 addition & 0 deletions src/constants/actionTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ const actionTypes = {
transactionCreatedSuccess: 'TRANSACTION_CREATED_SUCCESS',
transactionSignError: 'TRANSACTION_SIGN_ERROR',
transactionDoubleSigned: 'TRANSACTION_DOUBLE_SIGNED',
signatureSkipped: 'SIGNATURE_SKIPPED',
resetTransactionResult: 'RESET_TRANSACTION_RESULTS',
broadcastedTransactionSuccess: 'BROADCAST_TRANSACTION_SUCCESS',
broadcastedTransactionError: 'BROADCASTED_TRANSACTION_ERROR',
Expand Down
27 changes: 24 additions & 3 deletions src/store/actions/transactions.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ import {
} from '@constants';
import { isEmpty } from '@utils/helpers';
import { getTransactions, create, broadcast } from '@api/transaction';
import { signMultisigTransaction, transformTransaction } from '@utils/transaction';
import {
signMultisigTransaction,
transformTransaction,
createTransactionObject,
flattenTransaction,
} from '@utils/transaction';
import { extractKeyPair } from '@utils/account';
import { getTransactionSignatureStatus } from '@screens/signMultiSignTransaction/helpers';
import { timerReset } from './account';
Expand Down Expand Up @@ -228,7 +233,6 @@ export const transactionBroadcasted = transaction =>
* @param {object} data.rawTransaction Transaction config required by Lisk Element
* @param {object} data.sender
* @param {object} data.sender.data - Sender account info in Lisk API schema
* @todo account for privateKey and HW and increase test coverage once HW is implemented
*/
export const multisigTransactionSigned = ({
rawTransaction, sender,
Expand All @@ -241,7 +245,6 @@ export const multisigTransactionSigned = ({
passphrase: account.passphrase,
hwInfo: account.hwInfo,
};
// @todo move isTransactionFullySigned to a generic location
const txStatus = getTransactionSignatureStatus(sender.data, rawTransaction);

const [tx, error] = await signMultisigTransaction(
Expand All @@ -264,3 +267,21 @@ export const multisigTransactionSigned = ({
});
}
};

/**
* Used when a fully signed transaction is imported, this action
* skips the current user's signature and directly places the tx
* in the Redux store to be ready for broadcasting.
*
* @param {object} data
* @param {object} data.rawTransaction Transaction config required by Lisk Element
*/
export const signatureSkipped = ({ rawTransaction }) => {
const flatTx = flattenTransaction(rawTransaction);
const binaryTx = createTransactionObject(flatTx, rawTransaction.moduleAssetId);

return ({
type: actionTypes.signatureSkipped,
data: binaryTx,
});
};
30 changes: 30 additions & 0 deletions src/store/actions/transactions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
transactionBroadcasted,
transactionCreated,
multisigTransactionSigned,
signatureSkipped,
} from './transactions';
import { sampleTransaction } from '../../../test/constants/transactions';
import { getState } from '../../../test/fixtures/transactions';
Expand Down Expand Up @@ -409,4 +410,33 @@ describe('actions: transactions', () => {
expect(dispatch).toHaveBeenCalledWith(expectedAction);
});
});

describe('signatureSkipped', () => {
const props = {
rawTransaction: {
id: '9dc584c07c9d7ed77d54b6f8f43b8341c50e108cbcf582ceb3513388fe4ba84c',
moduleAssetId: '2:0',
fee: '10000000',
nonce: 0,
sender: {
publicKey: '00f046aea2782180c51f7271249a0c107e6b6295c6b3c31e43c1a3ed644dcdeb',
},
asset: {
amount: '200',
recipient: { address: 'lskz5r2nbgwrzjctbcffyrn8k74jdxdmd9cj9ng45' },
data: 'test',
},
signatures: [
'af975f7670394a1fe7eee4785c2a65f4604b1e7ad2e4367308738c86ed4837ace17960b32d6d7ceca2d5c2ae7709465bb69581d4ef2713ccef1d4ed4618c0d0a',
'ae67d1418797e48afae2c8d3641bd0ee4807e307d22df71a84d50555438a03c3df1f9162b2d7e54836979e78925261944e2c5aadaf9f9534b5cb8956fa95f501',
],
},
};

it('should return the tx ready to be broadcasted', () => {
expect(signatureSkipped(props)).toEqual(
expect.objectContaining({ type: actionTypes.signatureSkipped }),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to add data: binaryTx be added to the assertion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't perform any logic there and that part is actually extensively tested in utils/transaction.js. I think it's redundant, but if you still believe I have to test it I can add that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, one more thing, the use of flattenTransaction and createTransactionObject is really getting out of hand. We need a proper refactoring round there. I have to change all these tests anyways. Otherwise I don't know how we can handle the sapphire phase.

);
});
});
});
1 change: 1 addition & 0 deletions src/store/reducers/transactions.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ const transactions = (state = initialState, action) => { // eslint-disable-line

// Stored the signed transaction to be used for broadcasting or downloading
case actionTypes.transactionCreatedSuccess:
case actionTypes.signatureSkipped:
case actionTypes.transactionDoubleSigned:
return {
...state,
Expand Down
18 changes: 18 additions & 0 deletions src/store/reducers/transactions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,24 @@ describe('Reducer: transactions', () => {
});
});

describe('signatureSkipped', () => {
it('should store the signed transaction', () => {
const state = {
...defaultState,
signedTransaction: null,
};
const action = {
type: actionTypes.signatureSkipped,
data: mockTransactions[0],
};
const changedState = transactions(state, action);
expect(changedState).toEqual({
...defaultState,
signedTransaction: mockTransactions[0],
});
});
});

describe('transactionSignError', () => {
it('should store the signed creation/signature error', () => {
const state = {
Expand Down