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
Merged

Fix wallet view #6597

merged 13 commits into from
Oct 9, 2017

Conversation

ngotchac
Copy link
Contributor

@ngotchac ngotchac commented Sep 26, 2017

Closes #6672

The Multisig Wallet View wouldn't load if multiple transactions happened within one block. This PR fixes it.

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. M7-ui labels Sep 26, 2017
@@ -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

@@ -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
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

@ngotchac ngotchac removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Sep 27, 2017
@5chdn
Copy link
Contributor

5chdn commented Oct 4, 2017

1 Test fails.

1) WalletDetails renders defaults:
 Invariant Violation: Could not find "store" in either the context or props of "Connect(WalletDetails)". Either wrap the root component in a <Provider>, or explicitly pass "store" as a prop to "Connect(WalletDetails)".
  at invariant (node_modules/invariant/invariant.js:42:15)
  at new Connect (node_modules/react-redux/lib/components/connect.js:132:36)
  at node_modules/react-dom/lib/ReactCompositeComponent.js:295:18
  at measureLifeCyclePerf (node_modules/react-dom/lib/ReactCompositeComponent.js:75:12)
  at ShallowComponentWrapper._constructComponentWithoutOwner (node_modules/react-dom/lib/ReactCompositeComponent.js:294:16)
  at ShallowComponentWrapper.mountComponent (node_modules/react-dom/lib/ReactCompositeComponent.js:188:21)
  at Object.mountComponent (node_modules/react-dom/lib/ReactReconciler.js:46:35)
  at ReactShallowRenderer._render (node_modules/react-dom/lib/ReactShallowRenderer.js:126:23)
  at _batchedRender (node_modules/react-dom/lib/ReactShallowRenderer.js:79:12)
  at Object.batchedUpdates (node_modules/react-dom/lib/ReactDefaultBatchingStrategy.js:60:14)
  at Object.batchedUpdates (node_modules/react-dom/lib/ReactUpdates.js:97:27)
  at ReactShallowRenderer.render (node_modules/react-dom/lib/ReactShallowRenderer.js:106:18)
  at ReactShallowRenderer.render (node_modules/enzyme/build/react-compat.js:158:39)
  at node_modules/enzyme/build/ShallowWrapper.js:90:26
  at ReactDefaultBatchingStrategyTransaction.perform (node_modules/react-dom/lib/Transaction.js:140:20)
  at Object.batchedUpdates (node_modules/react-dom/lib/ReactDefaultBatchingStrategy.js:62:26)
  at batchedUpdates (node_modules/react-dom/lib/ReactUpdates.js:97:27)
  at node_modules/enzyme/build/ShallowWrapper.js:89:41
  at withSetStateAllowed (node_modules/enzyme/build/Utils.js:224:3)
  at new ShallowWrapper (node_modules/enzyme/build/ShallowWrapper.js:88:38)
  at shallow (node_modules/enzyme/build/shallow.js:19:10)
  at render (src/modals/CreateWallet/WalletDetails/walletDetails.spec.js:30:15)
  at Context.<anonymous> (src/modals/CreateWallet/WalletDetails/walletDetails.spec.js:47:12)

@5chdn 5chdn added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 5, 2017
@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Oct 9, 2017
@5chdn 5chdn added the B0-patch label Oct 9, 2017
@5chdn 5chdn added this to the 1.8 milestone Oct 9, 2017
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

@5chdn 5chdn mentioned this pull request Oct 9, 2017
@jacogr jacogr added the A8-looksgood 🦄 Pull request is reviewed well. label Oct 9, 2017
@jacogr jacogr removed the A0-pleasereview 🤓 Pull request needs code review. label Oct 9, 2017
@arkpar arkpar merged commit 8d1964b into master Oct 9, 2017
@arkpar arkpar deleted the ng-wallet-logs branch October 9, 2017 11:11
arkpar pushed a commit that referenced this pull request Oct 9, 2017
* Add safe fail for empty logs

* Filter transactions

* Add more logging

* Fix Wallet Creation and wallet tx list

* Remove logs

* Prevent selecting twice same wallet owner

* Fix tests

* Remove unused props

* Remove unused props
arkpar added a commit that referenced this pull request Oct 9, 2017
* Fix wallet view (#6597)

* Add safe fail for empty logs

* Filter transactions

* Add more logging

* Fix Wallet Creation and wallet tx list

* Remove logs

* Prevent selecting twice same wallet owner

* Fix tests

* Remove unused props

* Remove unused props

* Disallow pasting recovery phrases on first run (#6602)

* Fix disallowing paste of recovery phrase on first run, ref #6581

* Allow the leader of CATS pasting recovery phrases.

* Updated systemd files for linux (#6592)

Previous version put $BASE directory in root directory.
This version clearly explains how to run as root or as specific user.

Additional configuration:

* send SIGHUP for clean exit,

* restart on fail.

Tested on Ubuntu 16.04.3 LTS with 4.10.0-33-generic x86_64 kernel

* Don't expose port 80 for parity anymore (#6633)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forever loading transactions history on multi-sig
5 participants