-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Conversation
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
1 Test fails.
|
allowCopy: PropTypes.bool, | ||
allowedValues: PropTypes.array, |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
* 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
* 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)
Closes #6672
The Multisig Wallet View wouldn't load if multiple transactions happened within one block. This PR fixes it.