From 84bae60c0255049835ce2e14c8e3f8a4fac2f07c Mon Sep 17 00:00:00 2001 From: Daniel Ternyak Date: Mon, 29 Jan 2018 16:58:07 -0600 Subject: [PATCH] Refactor Generate Transaction (#960) * Make generic modal * Allow generic modals to be injected into send button component * Refactor generate transaction, cleanup transaction sagas, simplify signing process * Get passing unit tests * Make previous test more comprehensive * Fix ts errors --- .../transaction/actionCreators/sign.ts | 35 ++--- .../actions/transaction/actionTypes/sign.ts | 14 +- common/actions/transaction/constants.ts | 3 +- common/components/GenerateTransaction.tsx | 13 ++ .../GenerateTransaction/Container.tsx | 43 ------ .../components/GenerateTransaction/index.ts | 1 - .../GenerateTransactionFactory/Container.tsx | 20 +++ .../GenerateTransactionFactory.tsx} | 40 +++--- .../GenerateTransactionFactory/index.ts | 1 + common/reducers/transaction/sign/sign.ts | 10 +- .../sagas/transaction/current/currentValue.ts | 10 +- common/sagas/transaction/signing/helpers.ts | 11 +- common/sagas/transaction/signing/signing.ts | 14 +- .../transaction/current/currentValue.spec.ts | 122 +++++++++--------- 14 files changed, 160 insertions(+), 177 deletions(-) create mode 100644 common/components/GenerateTransaction.tsx delete mode 100644 common/components/GenerateTransaction/Container.tsx delete mode 100644 common/components/GenerateTransaction/index.ts create mode 100644 common/components/GenerateTransactionFactory/Container.tsx rename common/components/{GenerateTransaction/GenerateTransaction.tsx => GenerateTransactionFactory/GenerateTransactionFactory.tsx} (63%) create mode 100644 common/components/GenerateTransactionFactory/index.ts diff --git a/common/actions/transaction/actionCreators/sign.ts b/common/actions/transaction/actionCreators/sign.ts index 4de60e82..2ede9c25 100644 --- a/common/actions/transaction/actionCreators/sign.ts +++ b/common/actions/transaction/actionCreators/sign.ts @@ -1,9 +1,8 @@ import { SignTransactionFailedAction, - SignLocalTransactionRequestedAction, - SignWeb3TransactionRequestedAction, SignLocalTransactionSucceededAction, - SignWeb3TransactionSucceededAction + SignWeb3TransactionSucceededAction, + SignTransactionRequestedAction } from '../actionTypes'; import { TypeKeys } from '../constants'; @@ -12,6 +11,12 @@ const signTransactionFailed = (): SignTransactionFailedAction => ({ type: TypeKeys.SIGN_TRANSACTION_FAILED }); +type TSignTransactionRequested = typeof signTransactionRequested; +const signTransactionRequested = (payload: SignTransactionRequestedAction['payload']) => ({ + type: TypeKeys.SIGN_TRANSACTION_REQUESTED, + payload +}); + type TSignLocalTransactionSucceeded = typeof signLocalTransactionSucceeded; const signLocalTransactionSucceeded = ( payload: SignLocalTransactionSucceededAction['payload'] @@ -20,14 +25,6 @@ const signLocalTransactionSucceeded = ( payload }); -type TSignLocalTransactionRequested = typeof signLocalTransactionRequested; -const signLocalTransactionRequested = ( - payload: SignLocalTransactionRequestedAction['payload'] -): SignLocalTransactionRequestedAction => ({ - type: TypeKeys.SIGN_LOCAL_TRANSACTION_REQUESTED, - payload -}); - type TSignWeb3TransactionSucceeded = typeof signWeb3TransactionSucceeded; const signWeb3TransactionSucceeded = ( payload: SignWeb3TransactionSucceededAction['payload'] @@ -36,23 +33,13 @@ const signWeb3TransactionSucceeded = ( payload }); -type TSignWeb3TransactionRequested = typeof signWeb3TransactionRequested; -const signWeb3TransactionRequested = ( - payload: SignWeb3TransactionRequestedAction['payload'] -): SignWeb3TransactionRequestedAction => ({ - type: TypeKeys.SIGN_WEB3_TRANSACTION_REQUESTED, - payload -}); - export { + signTransactionRequested, signTransactionFailed, signLocalTransactionSucceeded, - signLocalTransactionRequested, signWeb3TransactionSucceeded, - signWeb3TransactionRequested, TSignLocalTransactionSucceeded, - TSignLocalTransactionRequested, TSignWeb3TransactionSucceeded, - TSignWeb3TransactionRequested, - TSignTransactionFailed + TSignTransactionFailed, + TSignTransactionRequested }; diff --git a/common/actions/transaction/actionTypes/sign.ts b/common/actions/transaction/actionTypes/sign.ts index d9c46c3c..f9bf5509 100644 --- a/common/actions/transaction/actionTypes/sign.ts +++ b/common/actions/transaction/actionTypes/sign.ts @@ -8,8 +8,8 @@ import { TypeKeys } from 'actions/transaction/constants'; */ /* Signing / Async actions */ -interface SignLocalTransactionRequestedAction { - type: TypeKeys.SIGN_LOCAL_TRANSACTION_REQUESTED; +interface SignTransactionRequestedAction { + type: TypeKeys.SIGN_TRANSACTION_REQUESTED; payload: EthTx; } interface SignLocalTransactionSucceededAction { @@ -17,10 +17,6 @@ interface SignLocalTransactionSucceededAction { payload: { signedTransaction: Buffer; indexingHash: string; noVerify?: boolean }; // dont verify against fields, for pushTx } -interface SignWeb3TransactionRequestedAction { - type: TypeKeys.SIGN_WEB3_TRANSACTION_REQUESTED; - payload: EthTx; -} interface SignWeb3TransactionSucceededAction { type: TypeKeys.SIGN_WEB3_TRANSACTION_SUCCEEDED; payload: { transaction: Buffer; indexingHash: string; noVerify?: boolean }; @@ -30,16 +26,14 @@ interface SignTransactionFailedAction { } type SignAction = - | SignLocalTransactionRequestedAction + | SignTransactionRequestedAction | SignLocalTransactionSucceededAction - | SignWeb3TransactionRequestedAction | SignWeb3TransactionSucceededAction | SignTransactionFailedAction; export { - SignLocalTransactionRequestedAction, + SignTransactionRequestedAction, SignLocalTransactionSucceededAction, - SignWeb3TransactionRequestedAction, SignWeb3TransactionSucceededAction, SignTransactionFailedAction, SignAction diff --git a/common/actions/transaction/constants.ts b/common/actions/transaction/constants.ts index 4d08f6c0..ba38d15e 100644 --- a/common/actions/transaction/constants.ts +++ b/common/actions/transaction/constants.ts @@ -12,9 +12,8 @@ export enum TypeKeys { GET_NONCE_SUCCEEDED = 'GET_NONCE_SUCCEEDED', GET_NONCE_FAILED = 'GET_NONCE_FAILED', - SIGN_WEB3_TRANSACTION_REQUESTED = 'SIGN_WEB3_TRANSACTION_REQUESTED', + SIGN_TRANSACTION_REQUESTED = 'SIGN_TRANSACTION_REQUESTED', SIGN_WEB3_TRANSACTION_SUCCEEDED = 'SIGN_WEB3_TRANSACTION_SUCCEEDED', - SIGN_LOCAL_TRANSACTION_REQUESTED = 'SIGN_LOCAL_TRANSACTION_REQUESTED', SIGN_LOCAL_TRANSACTION_SUCCEEDED = 'SIGN_LOCAL_TRANSACTION_SUCCEEDED', SIGN_TRANSACTION_FAILED = 'SIGN_TRANSACTION_FAILED', diff --git a/common/components/GenerateTransaction.tsx b/common/components/GenerateTransaction.tsx new file mode 100644 index 00000000..c9b2fed8 --- /dev/null +++ b/common/components/GenerateTransaction.tsx @@ -0,0 +1,13 @@ +import { GenerateTransactionFactory } from './GenerateTransactionFactory'; +import React from 'react'; +import translate from 'translations'; + +export const GenerateTransaction: React.SFC<{}> = () => ( + ( + + )} + /> +); diff --git a/common/components/GenerateTransaction/Container.tsx b/common/components/GenerateTransaction/Container.tsx deleted file mode 100644 index 52592b90..00000000 --- a/common/components/GenerateTransaction/Container.tsx +++ /dev/null @@ -1,43 +0,0 @@ -import { - signLocalTransactionRequested, - signWeb3TransactionRequested, - TSignLocalTransactionRequested, - TSignWeb3TransactionRequested, - SignLocalTransactionRequestedAction, - SignWeb3TransactionRequestedAction -} from 'actions/transaction'; -import React, { Component } from 'react'; -import { connect, Dispatch } from 'react-redux'; -import { bindActionCreators } from 'redux'; -import { AppState } from 'reducers'; -type Payload = - | SignLocalTransactionRequestedAction['payload'] - | SignWeb3TransactionRequestedAction['payload']; -type Signer = ( - payload: Payload -) => () => SignLocalTransactionRequestedAction | SignWeb3TransactionRequestedAction; - -interface DispatchProps { - signer: TSignLocalTransactionRequested | TSignWeb3TransactionRequested; -} -interface Props { - isWeb3: boolean; - withSigner(signer: Signer): React.ReactElement | null; -} - -class Container extends Component { - public render() { - return this.props.withSigner(this.sign); - } - - private sign = (payload: Payload) => () => this.props.signer(payload); -} - -export const WithSigner = connect(null, (dispatch: Dispatch, ownProps: Props) => { - return bindActionCreators( - { - signer: ownProps.isWeb3 ? signWeb3TransactionRequested : signLocalTransactionRequested - }, - dispatch - ); -})(Container); diff --git a/common/components/GenerateTransaction/index.ts b/common/components/GenerateTransaction/index.ts deleted file mode 100644 index ad58d799..00000000 --- a/common/components/GenerateTransaction/index.ts +++ /dev/null @@ -1 +0,0 @@ -export * from './GenerateTransaction'; diff --git a/common/components/GenerateTransactionFactory/Container.tsx b/common/components/GenerateTransactionFactory/Container.tsx new file mode 100644 index 00000000..d7814ead --- /dev/null +++ b/common/components/GenerateTransactionFactory/Container.tsx @@ -0,0 +1,20 @@ +import { signTransactionRequested, TSignTransactionRequested } from 'actions/transaction'; +import React, { Component } from 'react'; +import { connect } from 'react-redux'; + +interface DispatchProps { + signTransactionRequested: TSignTransactionRequested; +} + +interface OwnProps { + isWeb3: boolean; + withSigner(signer: TSignTransactionRequested): React.ReactElement | null; +} + +class Container extends Component { + public render() { + return this.props.withSigner(this.props.signTransactionRequested); + } +} + +export const WithSigner = connect(null, { signTransactionRequested })(Container); diff --git a/common/components/GenerateTransaction/GenerateTransaction.tsx b/common/components/GenerateTransactionFactory/GenerateTransactionFactory.tsx similarity index 63% rename from common/components/GenerateTransaction/GenerateTransaction.tsx rename to common/components/GenerateTransactionFactory/GenerateTransactionFactory.tsx index 724a9e2f..9a5c0eb0 100644 --- a/common/components/GenerateTransaction/GenerateTransaction.tsx +++ b/common/components/GenerateTransactionFactory/GenerateTransactionFactory.tsx @@ -1,4 +1,3 @@ -import translate from 'translations'; import { WithSigner } from './Container'; import EthTx from 'ethereumjs-tx'; import React, { Component } from 'react'; @@ -12,6 +11,12 @@ import { } from 'selectors/transaction'; import { getWalletType } from 'selectors/wallet'; +export interface CallbackProps { + disabled: boolean; + isWeb3Wallet: boolean; + onClick(): void; +} + interface StateProps { transaction: EthTx; networkRequestPending: boolean; @@ -21,16 +26,21 @@ interface StateProps { validGasLimit: boolean; } -class GenerateTransactionClass extends Component { +interface OwnProps { + withProps(props: CallbackProps): React.ReactElement | null; +} + +type Props = OwnProps & StateProps; + +class GenerateTransactionFactoryClass extends Component { public render() { const { isFullTransaction, isWeb3Wallet, - transaction, networkRequestPending, - validGasPrice, - validGasLimit + validGasLimit, + transaction } = this.props; const isButtonDisabled = @@ -38,24 +48,22 @@ class GenerateTransactionClass extends Component { return ( ( - - )} + withSigner={signer => + this.props.withProps({ + disabled: isButtonDisabled, + isWeb3Wallet, + onClick: () => signer(transaction) + }) + } /> ); } } -export const GenerateTransaction = connect((state: AppState) => ({ +export const GenerateTransactionFactory = connect((state: AppState) => ({ ...getTransaction(state), networkRequestPending: isNetworkRequestPending(state), isWeb3Wallet: getWalletType(state).isWeb3Wallet, validGasPrice: isValidGasPrice(state), validGasLimit: isValidGasLimit(state) -}))(GenerateTransactionClass); +}))(GenerateTransactionFactoryClass); diff --git a/common/components/GenerateTransactionFactory/index.ts b/common/components/GenerateTransactionFactory/index.ts new file mode 100644 index 00000000..e93983bd --- /dev/null +++ b/common/components/GenerateTransactionFactory/index.ts @@ -0,0 +1 @@ +export * from './GenerateTransactionFactory'; diff --git a/common/reducers/transaction/sign/sign.ts b/common/reducers/transaction/sign/sign.ts index fb53b233..cb42ca0a 100644 --- a/common/reducers/transaction/sign/sign.ts +++ b/common/reducers/transaction/sign/sign.ts @@ -14,7 +14,7 @@ const INITIAL_STATE: State = { pending: false }; -const signLocalTransactionRequested = (): State => ({ +const signTransactionRequested = (): State => ({ ...INITIAL_STATE, pending: true }); @@ -30,7 +30,7 @@ const signLocalTransactionSucceeded = ( web3: { transaction: null } }); -const signWeb3TranscationRequested = ( +const signWeb3TranscationSucceeded = ( _: State, { payload }: SignWeb3TransactionSucceededAction ): State => ({ @@ -47,12 +47,12 @@ const reset = () => INITIAL_STATE; export const sign = (state: State = INITIAL_STATE, action: SignAction | ResetAction) => { switch (action.type) { - case TK.SIGN_LOCAL_TRANSACTION_REQUESTED: - return signLocalTransactionRequested(); + case TK.SIGN_TRANSACTION_REQUESTED: + return signTransactionRequested(); case TK.SIGN_LOCAL_TRANSACTION_SUCCEEDED: return signLocalTransactionSucceeded(state, action); case TK.SIGN_WEB3_TRANSACTION_SUCCEEDED: - return signWeb3TranscationRequested(state, action); + return signWeb3TranscationSucceeded(state, action); case TK.SIGN_TRANSACTION_FAILED: return signTransactionFailed(); case TK.RESET: diff --git a/common/sagas/transaction/current/currentValue.ts b/common/sagas/transaction/current/currentValue.ts index 338076c3..bb20403a 100644 --- a/common/sagas/transaction/current/currentValue.ts +++ b/common/sagas/transaction/current/currentValue.ts @@ -12,11 +12,16 @@ import { SetCurrentValueAction, TypeKeys } from 'actions/transaction'; import { toTokenBase } from 'libs/units'; import { validateInput, IInput } from 'sagas/transaction/validationHelpers'; import { validNumber, validDecimal } from 'libs/validators'; -export function* setCurrentValue({ payload }: SetCurrentValueAction): SagaIterator { + +export function* setCurrentValue(action: SetCurrentValueAction): SagaIterator { const etherTransaction = yield select(isEtherTransaction); + const setter = etherTransaction ? setValueField : setTokenValue; + return yield call(valueHandler, action, setter); +} + +export function* valueHandler({ payload }: SetCurrentValueAction, setter) { const decimal: number = yield select(getDecimal); const unit: string = yield select(getUnit); - const setter = etherTransaction ? setValueField : setTokenValue; if (!validNumber(+payload) || !validDecimal(payload, decimal)) { return yield put(setter({ raw: payload, value: null })); @@ -51,6 +56,7 @@ export function* reparseCurrentValue(value: IInput): SagaIterator { return null; } } + export const currentValue = [ takeEvery([TypeKeys.CURRENT_VALUE_SET], setCurrentValue), takeEvery([TypeKeys.GAS_LIMIT_FIELD_SET, TypeKeys.GAS_PRICE_FIELD_SET], revalidateCurrentValue) diff --git a/common/sagas/transaction/signing/helpers.ts b/common/sagas/transaction/signing/helpers.ts index a13de4ec..307f54c6 100644 --- a/common/sagas/transaction/signing/helpers.ts +++ b/common/sagas/transaction/signing/helpers.ts @@ -4,12 +4,11 @@ import { getNetworkConfig } from 'selectors/config'; import { select, call, put, take } from 'redux-saga/effects'; import { signTransactionFailed, - SignWeb3TransactionRequestedAction, - SignLocalTransactionRequestedAction, GetFromFailedAction, GetFromSucceededAction, getFromRequested, - TypeKeys as TK + TypeKeys as TK, + SignTransactionRequestedAction } from 'actions/transaction'; import Tx from 'ethereumjs-tx'; import { NetworkConfig } from 'config'; @@ -22,7 +21,7 @@ interface IFullWalletAndTransaction { } const signTransactionWrapper = (func: (IWalletAndTx: IFullWalletAndTransaction) => SagaIterator) => - function*(partialTx: SignLocalTransactionRequestedAction | SignWeb3TransactionRequestedAction) { + function*(partialTx: SignTransactionRequestedAction) { try { const IWalletAndTx: IFullWalletAndTransaction = yield call( getWalletAndTransaction, @@ -40,9 +39,7 @@ const signTransactionWrapper = (func: (IWalletAndTx: IFullWalletAndTransaction) * the rest of the tx parameters from the action * @param partialTx */ -function* getWalletAndTransaction( - partialTx: (SignLocalTransactionRequestedAction | SignWeb3TransactionRequestedAction)['payload'] -) { +function* getWalletAndTransaction(partialTx: SignTransactionRequestedAction['payload']) { // get the wallet we're going to sign with const wallet: null | IFullWallet = yield select(getWalletInst); if (!wallet) { diff --git a/common/sagas/transaction/signing/signing.ts b/common/sagas/transaction/signing/signing.ts index beb8d0ed..6121e115 100644 --- a/common/sagas/transaction/signing/signing.ts +++ b/common/sagas/transaction/signing/signing.ts @@ -7,11 +7,13 @@ import { TypeKeys, reset, SignWeb3TransactionSucceededAction, - SignLocalTransactionSucceededAction + SignLocalTransactionSucceededAction, + SignTransactionRequestedAction } from 'actions/transaction'; import { computeIndexingHash } from 'libs/transaction'; import { serializedAndTransactionFieldsMatch } from 'selectors/transaction'; import { showNotification } from 'actions/notifications'; +import { getWalletType, IWalletType } from 'selectors/wallet'; export function* signLocalTransactionHandler({ tx, @@ -64,9 +66,15 @@ function* verifyTransaction({ yield put(reset()); } } + +function* handleTransactionRequest(action: SignTransactionRequestedAction): SagaIterator { + const walletType: IWalletType = yield select(getWalletType); + const signingHandler = walletType.isWeb3Wallet ? signWeb3Transaction : signLocalTransaction; + return yield call(signingHandler, action); +} + export const signing = [ - takeEvery(TypeKeys.SIGN_LOCAL_TRANSACTION_REQUESTED, signLocalTransaction), - takeEvery(TypeKeys.SIGN_WEB3_TRANSACTION_REQUESTED, signWeb3Transaction), + takeEvery(TypeKeys.SIGN_TRANSACTION_REQUESTED, handleTransactionRequest), takeEvery( [TypeKeys.SIGN_LOCAL_TRANSACTION_SUCCEEDED, TypeKeys.SIGN_WEB3_TRANSACTION_SUCCEEDED], verifyTransaction diff --git a/spec/sagas/transaction/current/currentValue.spec.ts b/spec/sagas/transaction/current/currentValue.spec.ts index ba705ac3..b5591386 100644 --- a/spec/sagas/transaction/current/currentValue.spec.ts +++ b/spec/sagas/transaction/current/currentValue.spec.ts @@ -6,8 +6,10 @@ import { validateInput } from 'sagas/transaction/validationHelpers'; import { setCurrentValue, revalidateCurrentValue, - reparseCurrentValue + reparseCurrentValue, + valueHandler } from 'sagas/transaction/current/currentValue'; +import { cloneableGenerator, SagaIteratorClone } from 'redux-saga/utils'; const itShouldBeDone = gen => { it('should be done', () => { @@ -15,77 +17,69 @@ const itShouldBeDone = gen => { }); }; -describe('setCurrentValue*', () => { - const sharedLogic = (gen, etherTransaction, decimal: number) => { - it('should select isEtherTransaction', () => { - expect(gen.next().value).toEqual(select(isEtherTransaction)); - }); +describe('valueHandler', () => { + const action: any = { payload: '5.1' }; + const setter = setValueField; + const decimal = 1; + const gen: { [key: string]: SagaIteratorClone } = {}; - it('should select getDecimal', () => { - expect(gen.next(etherTransaction).value).toEqual(select(getDecimal)); - }); - it('should select getUnit', () => { - expect(gen.next(decimal).value).toEqual(select(getUnit)); - }); + const failCases = { + invalidDecimal: 0, + invalidNumber: { + decimal: 1, + action: { payload: 'x' } + } }; - describe('when invalid number or decimal', () => { - const invalidDecimal: any = { - payload: '10.01' - }; - const invalidNumber: any = { - payload: 'invalidNumber' - }; - const etherTransaction = true; - const decimal = 1; - const gen1 = setCurrentValue(invalidNumber); - const gen2 = setCurrentValue(invalidDecimal); + gen.pass = cloneableGenerator(valueHandler)(action, setter); + gen.invalidNumber = cloneableGenerator(valueHandler)( + failCases.invalidNumber.action as any, + setter + ); + const value = toTokenBase(action.payload, decimal); + const unit = 'eth'; - sharedLogic(gen2, etherTransaction, decimal); - sharedLogic(gen1, etherTransaction, decimal); + it('should select getDecimal', () => { + expect(gen.pass.next().value).toEqual(select(getDecimal)); + expect(gen.invalidNumber.next().value).toEqual(select(getDecimal)); + }); + it('should select getUnit', () => { + gen.invalidDecimal = gen.pass.clone(); - it('should put setter', () => { - expect(gen1.next().value).toEqual( - put(setValueField({ raw: invalidNumber.payload, value: null })) - ); - expect(gen2.next().value).toEqual( - put(setValueField({ raw: invalidDecimal.payload, value: null })) - ); - }); - - itShouldBeDone(gen1); - itShouldBeDone(gen2); + expect(gen.pass.next(decimal).value).toEqual(select(getUnit)); + expect(gen.invalidNumber.next(decimal).value).toEqual(select(getUnit)); + expect(gen.invalidDecimal.next(failCases.invalidDecimal).value).toEqual(select(getUnit)); }); - describe('when valid number and decimal', () => { - const payload = '100'; - const action: any = { payload }; - const etherTransaction = true; - const unit = 'ether'; - const decimal = 0; - const value = toTokenBase(payload, decimal); - const isValid = true; - - const gen = setCurrentValue(action); - - sharedLogic(gen, etherTransaction, decimal); - it('should call validateInput', () => { - expect(gen.next(unit).value).toEqual(call(validateInput, value, unit)); - }); - - it('should put setter', () => { - expect(gen.next(isValid).value).toEqual( - put( - setValueField({ - raw: payload, - value - }) - ) - ); - }); - - itShouldBeDone(gen); + it('should fail on invalid number or decimal and put null as a value', () => { + expect(gen.invalidNumber.next(unit).value).toEqual( + put(setter({ raw: failCases.invalidNumber.action.payload, value: null })) + ); + expect(gen.invalidDecimal.next(unit).value).toEqual( + put(setter({ raw: action.payload, value: null })) + ); }); + + it('should call isValid', () => { + expect(gen.pass.next(unit).value).toEqual(call(validateInput, value, unit)); + }); + it('should put setter', () => { + expect(gen.pass.next(true).value).toEqual(put(setter({ raw: action.payload, value }))); + }); + + itShouldBeDone(gen.pass); +}); + +describe('setCurrentValue*', () => { + const action: any = { payload: '5' }; + const gen = setCurrentValue(action); + it('should select isEtherTransaction', () => { + expect(gen.next().value).toEqual(select(isEtherTransaction)); + }); + it('should call valueHandler', () => { + expect(gen.next(isEtherTransaction).value).toEqual(call(valueHandler, action, setValueField)); + }); + itShouldBeDone(gen); }); describe('revalidateCurrentValue*', () => {