Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fix wallet view #6597

Merged
merged 13 commits into from
Oct 9, 2017
32 changes: 28 additions & 4 deletions js/src/modals/CreateWallet/WalletDetails/walletDetails.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,21 @@

import React, { Component, PropTypes } from 'react';
import { FormattedMessage } from 'react-intl';
import { connect } from 'react-redux';

import { Form, TypedInput, Input, AddressSelect, InputAddress } from '~/ui';

import styles from '../createWallet.css';

export default class WalletDetails extends Component {
class WalletDetails extends Component {
static propTypes = {
accounts: PropTypes.object.isRequired,
wallet: PropTypes.object.isRequired,
errors: PropTypes.object.isRequired,
onChange: PropTypes.func.isRequired,
walletType: PropTypes.string.isRequired
walletType: PropTypes.string.isRequired,

knownAddresses: PropTypes.array
};

render () {
Expand Down Expand Up @@ -103,7 +106,10 @@ export default class WalletDetails extends Component {
}

renderMultisigDetails () {
const { accounts, wallet, errors } = this.props;
const { accounts, knownAddresses, wallet, errors } = this.props;
const allowedOwners = knownAddresses
// Exclude sender and already owners of the wallet
.filter((address) => !wallet.owners.includes(address) && address !== wallet.account);

return (
<Form>
Expand Down Expand Up @@ -163,7 +169,7 @@ export default class WalletDetails extends Component {
/>

<TypedInput
accounts={ accounts }
allowedValues={ allowedOwners }
label={
<FormattedMessage
id='createWallet.details.ownersMulti.label'
Expand Down Expand Up @@ -249,3 +255,21 @@ export default class WalletDetails extends Component {
this.props.onChange({ daylimit });
}
}

function mapStateToProps (initState) {
const { accounts, contacts, contracts } = initState.personal;
const knownAddresses = [].concat(
Object.keys(accounts),
Object.keys(contacts),
Object.keys(contracts)
);

return () => ({
knownAddresses
});
}

export default connect(
mapStateToProps,
null
)(WalletDetails);
23 changes: 22 additions & 1 deletion js/src/modals/CreateWallet/WalletDetails/walletDetails.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,22 @@ import { ACCOUNTS } from '../createWallet.test.js';
let component;
let onChange;

function createRedux () {
return {
dispatch: sinon.stub(),
subscribe: sinon.stub(),
getState: () => {
return {
personal: {
accounts: {},
contacts: {},
contracts: {}
}
};
}
};
}

function render (walletType = 'MULTISIG') {
onChange = sinon.stub();
component = shallow(
Expand All @@ -36,7 +52,12 @@ function render (walletType = 'MULTISIG') {
owners: []
} }
walletType={ walletType }
/>
/>,
{
context: {
store: createRedux()
}
}
);

return component;
Expand Down
3 changes: 2 additions & 1 deletion js/src/modals/CreateWallet/createWalletStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,8 @@ export default class CreateWalletStore {

const owners = _wallet.owners.filter((owner) => !/^(0x)?0*$/.test(owner));

if (_wallet.required > owners.length) {
// Real number of owners is owners + creator
Copy link
Contributor

Choose a reason for hiding this comment

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

is this true for the latest wallet code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay so it seems that it hasn't been updated to the latest wallet (after the last PR was merged in paritytech/contracts), so msg.sender is still one of the owner.
We should update it to the latest version. Although it would be easier with this PR merged : https://github.com/paritytech/contracts/pull/74

if (_wallet.required > owners.length + 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should now check that there are no duplicate identities between the creator and named owners.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the actual wallet code ensuring that?

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure about the very latest version, but i know at least one of the original versions didn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least here it doesn't : https://github.com/paritytech/contracts/blob/master/Wallet.sol#L58
We could enforce this UI-wise though, even if it's not enforced in the underlying contract.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

requiredValidation.valueError = 'the number of required validators should be lower or equal the number of owners';
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ export default class ParametersStep extends Component {
};

static propTypes = {
accounts: PropTypes.object.isRequired,
onParamsChange: PropTypes.func.isRequired,

inputs: PropTypes.array,
Expand All @@ -60,7 +59,7 @@ export default class ParametersStep extends Component {
}

renderConstructorInputs () {
const { accounts, params, paramsError } = this.props;
const { params, paramsError } = this.props;
const { inputs } = this.props;

if (!inputs || !inputs.length) {
Expand All @@ -78,7 +77,6 @@ export default class ParametersStep extends Component {
return (
<div key={ index } className={ styles.funcparams }>
<TypedInput
accounts={ accounts }
error={ error }
isEth={ false }
label={ label }
Expand Down
1 change: 0 additions & 1 deletion js/src/modals/DeployContract/deployContract.js
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,6 @@ class DeployContract extends Component {
return (
<ParametersStep
{ ...this.state }
accounts={ accounts }
onParamsChange={ this.onParamsChange }
readOnly={ readOnly }
/>
Expand Down
3 changes: 1 addition & 2 deletions js/src/modals/ExecuteContract/DetailsStep/detailsStep.js
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ export default class DetailsStep extends Component {
}

renderParameters () {
const { accounts, func, values, valuesError, onValueChange } = this.props;
const { func, values, valuesError, onValueChange } = this.props;

if (!func) {
return null;
Expand All @@ -197,7 +197,6 @@ export default class DetailsStep extends Component {
value={ values[index] }
error={ valuesError[index] }
onChange={ onChange }
accounts={ accounts }
param={ input.type }
isEth={ false }
/>
Expand Down
8 changes: 3 additions & 5 deletions js/src/modals/WalletSettings/walletSettings.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ class WalletSettings extends Component {
};

static propTypes = {
accountsInfo: PropTypes.object.isRequired,
wallet: PropTypes.object.isRequired,
onClose: PropTypes.func.isRequired,
senders: PropTypes.object.isRequired
Expand Down Expand Up @@ -74,7 +73,7 @@ class WalletSettings extends Component {
default:
case 'EDIT':
const { errors, fromString, wallet } = this.store;
const { accountsInfo, senders } = this.props;
const { senders } = this.props;

return (
<Form>
Expand Down Expand Up @@ -143,7 +142,6 @@ class WalletSettings extends Component {
}
value={ wallet.owners.slice() }
onChange={ this.store.onOwnersChange }
accounts={ accountsInfo }
param='address[]'
/>

Expand Down Expand Up @@ -443,13 +441,13 @@ class WalletSettings extends Component {
}

function mapStateToProps (initState, initProps) {
const { accountsInfo, accounts } = initState.personal;
const { accounts } = initState.personal;
const { owners } = initProps.wallet;

const senders = pick(accounts, owners);

return () => {
return { accountsInfo, senders };
return { senders };
};
}

Expand Down
13 changes: 13 additions & 0 deletions js/src/ui/Form/AddressSelect/addressSelect.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// You should have received a copy of the GNU General Public License
// along with Parity. If not, see <http://www.gnu.org/licenses/>.

import { eq } from 'lodash';
import React, { Component, PropTypes } from 'react';
import ReactDOM from 'react-dom';
import { connect } from 'react-redux';
Expand Down Expand Up @@ -93,6 +94,18 @@ class AddressSelect extends Component {
}

componentWillReceiveProps (nextProps) {
if (!eq(Object.keys(this.props.accounts), Object.keys(nextProps.accounts))) {
return this.setValues(nextProps);
}

if (!eq(Object.keys(this.props.contacts), Object.keys(nextProps.contacts))) {
return this.setValues(nextProps);
}

if (!eq(Object.keys(this.props.contracts), Object.keys(nextProps.contracts))) {
return this.setValues(nextProps);
}

if (this.store.values && this.store.values.length > 0) {
return;
}
Expand Down
3 changes: 2 additions & 1 deletion js/src/ui/Form/AddressSelect/addressSelectStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,8 @@ export default class AddressSelectStore {
const contactsN = Object.keys(contacts).length;

if (accountsN + contractsN + contactsN === 0) {
return;
this.initValues = [];
return this.handleChange();
}

this.initValues = [
Expand Down
27 changes: 23 additions & 4 deletions js/src/ui/Form/InputAddressSelect/inputAddressSelect.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// You should have received a copy of the GNU General Public License
// along with Parity. If not, see <http://www.gnu.org/licenses/>.

import { pick } from 'lodash';
import React, { Component, PropTypes } from 'react';
import { connect } from 'react-redux';

Expand All @@ -28,6 +29,7 @@ class InputAddressSelect extends Component {
contracts: PropTypes.object.isRequired,

allowCopy: PropTypes.bool,
allowedValues: PropTypes.array,
className: PropTypes.string,
error: nodeOrStringProptype(),
hint: nodeOrStringProptype(),
Expand All @@ -38,16 +40,33 @@ class InputAddressSelect extends Component {
};

render () {
const { accounts, allowCopy, className, contacts, contracts, label, hint, error, value, onChange, readOnly } = this.props;
const { accounts, allowCopy, allowedValues, className, contacts, contracts, label, hint, error, value, onChange, readOnly } = this.props;
// Add the currently selected value to the list
// of allowed values, if any given
const nextAllowedValues = allowedValues
? [].concat(allowedValues, value || [])
: null;

const filteredAccounts = nextAllowedValues
? pick(accounts, nextAllowedValues)
: accounts;

const filteredContacts = nextAllowedValues
? pick(contacts, nextAllowedValues)
: accounts;

const filteredContracts = nextAllowedValues
? pick(contracts, nextAllowedValues)
: accounts;

return (
<AddressSelect
allowCopy={ allowCopy }
allowInput
accounts={ accounts }
accounts={ filteredAccounts }
className={ className }
contacts={ contacts }
contracts={ contracts }
contacts={ filteredContacts }
contracts={ filteredContracts }
error={ error }
hint={ hint }
label={ label }
Expand Down
10 changes: 5 additions & 5 deletions js/src/ui/Form/TypedInput/typedInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ export default class TypedInput extends Component {
PropTypes.string
]).isRequired,

accounts: PropTypes.object,
allowCopy: PropTypes.bool,
allowedValues: PropTypes.array,
Copy link
Contributor

Choose a reason for hiding this comment

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

DeployContract also used TypedInput, don't see changes addressing the changed interface? (As well as Contract queries, Mining Settings - all passing accounts instead of allowedValues to TypedInput?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a required prop. If empty, it should be ignored (ie. all values are allowed)

Copy link
Contributor

@jacogr jacogr Oct 9, 2017

Choose a reason for hiding this comment

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

Perfect, however we are still passing accounts through at various places now and assuming (from the caller's perspective) it does get used, otherwise the caller shouldn't provide it.

If those are needed, should be replaced with the real values, if not, should be removed since it creates confusion in the codebase. (And warnings when running)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

className: PropTypes.string,
error: PropTypes.any,
hint: nodeOrStringProptype(),
Expand Down Expand Up @@ -97,7 +97,7 @@ export default class TypedInput extends Component {
const { type } = param;

if (type === ABI_TYPES.ARRAY) {
const { accounts, className, label } = this.props;
const { allowedValues, className, label } = this.props;
const { subtype, length } = param;
const value = this.getValue() || param.default;

Expand All @@ -113,8 +113,8 @@ export default class TypedInput extends Component {

return (
<TypedInput
accounts={ accounts }
allowCopy={ allowCopy }
allowedValues={ allowedValues }
className={ className }
key={ `${subtype.type}_${index}` }
onChange={ onChange }
Expand Down Expand Up @@ -340,13 +340,13 @@ export default class TypedInput extends Component {
}

renderAddress () {
const { accounts, allowCopy, className, label, error, hint, readOnly } = this.props;
const { allowCopy, allowedValues, className, label, error, hint, readOnly } = this.props;
const value = this.getValue();

return (
<InputAddressSelect
allowCopy={ allowCopy }
accounts={ accounts }
allowedValues={ allowedValues }
className={ className }
error={ error }
hint={ hint }
Expand Down
30 changes: 28 additions & 2 deletions js/src/util/wallets.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,39 @@ export default class WalletsUtils {
.delegateCall(api, walletContract.address, 'fetchTransactions', [ walletContract ])
.then((transactions) => {
return transactions.sort((txA, txB) => {
const comp = txB.blockNumber.comparedTo(txA.blockNumber);
const bnA = txA.blockNumber;
const bnB = txB.blockNumber;

if (!bnA) {
console.warn('could not find block number in transaction', txA);
return 1;
}

if (!bnB) {
console.warn('could not find block number in transaction', txB);
return -1;
}

const comp = bnA.comparedTo(bnB);

if (comp !== 0) {
return comp;
}

return txB.transactionIndex.comparedTo(txA.transactionIndex);
const txIdxA = txA.transactionIndex;
const txIdxB = txB.transactionIndex;

if (!txIdxA) {
console.warn('could not find transaction index in transaction', txA);
return 1;
}

if (!txIdxB) {
console.warn('could not find transaction index in transaction', txB);
return -1;
}

return txIdxA.comparedTo(txIdxB);
});
});
}
Expand Down
1 change: 1 addition & 0 deletions js/src/util/wallets/consensys-wallet.js
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ export default class ConsensysWalletUtils {

const transaction = {
transactionHash: log.transactionHash,
transactionIndex: log.transactionIndex,
blockNumber: log.blockNumber
};

Expand Down
Loading