Make Wallet first-class citizens #3990

ngotchac · 2016-12-28T16:34:00Z

This PR fixes some issues (balances not updating, not being able to send tokens from a Wallet), and should close #3784

This adds a new lookup action to the Contract class, which checks for a transaction whether the from field corresponds to a Wallet or not. If it does, then try to post the transaction/estimate gas with the correct options, ie. from being one of the owner of the wallet that is also one of the user account, etc.
Note that this can be recursive : if a Wallet is owned by another Wallet owner by one of my account, it will correctly set the account as the transaction sender, to the middle Wallet, that will call the execute method on the initial Wallet, which will execute the correct transaction.

The gas estimation is sometimes (more or less 2/3) wrong. So to test one might want to multiply the gas by 2. Maybe this should be taken into account in the PR (while the gas estimation gets fixed) ?

As the modification lies in the Contract class, this also work within dApps ! The only thing not available for Wallets is contract deployment (that would require a modification of the Wallet contract itself)...

Might fix #3849

gavofyork · 2016-12-28T17:09:32Z

cool - we should probably add a create function to our standard wallet contracts. how easily will this play with DeadMansSwitch?

coveralls · 2016-12-28T17:22:43Z

Coverage remained the same at 86.601% when pulling c307fcc on ng-wallet-improv into 3067a8d on master.

ngotchac · 2016-12-28T17:31:49Z

@gavofyork So the DeadManSwitch contract is working fine when backup/owner are Wallets Contracts (I just tested every methods), from the Contracts view. However, the DeadManSwitch contract cannot be used as a Wallet for now. I think we might want to open the Wallet options to other contracts than the standard one. However it would take some work to get there, as we need to make the events and methods that we use more general.

jacogr · 2016-12-29T08:47:41Z

js/src/redux/providers/signerReducer.js

@@ -90,7 +90,7 @@ export default handleActions({
  signerSuccessRejectRequest (state, action) {
    const { id } = action.payload;
    const rejected = Object.assign(
-      state.pending.find(p => p.id === id),
+      state.pending.find(p => p.id === id) || { id },


Same as the previous fix (now for reject), I missed this case the first time around. (Let's hope it is indeed dead now)

jacogr · 2016-12-29T08:48:28Z

js/src/redux/providers/personalReducer.js

-      hasContracts: Object.keys(contracts).length !== 0,
-      wallets,
-      hasWallets: Object.keys(wallets).length !== 0
+      hasContracts: Object.keys(contracts).length !== 0


While we are here, could we just fix the ordering as well?

jacogr · 2016-12-29T08:50:25Z

js/src/api/contract/contract.js

+   * a Wallet. The result is cached in order not
+   * to make unnecessary calls on non wallet accounts
+   */
+  _isWallet (address) {


Not 100% sure this is the correct place for this functionality. Can we walk through today and discuss?

jacogr · 2016-12-29T09:00:38Z

...onents/TransactionPendingForm/TransactionPendingFormConfirm/transactionPendingFormConfirm.js

@@ -60,6 +60,7 @@ class TransactionPendingFormConfirm extends Component {
        <Form>
          { keyInput }
          <Input
+            autoFocus


Where does it focus when there are multiple requests? Still on the first?

jacogr · 2016-12-29T09:02:11Z

js/src/views/Wallet/wallet.js

    wallet: PropTypes.object.isRequired,
-    isTest: PropTypes.bool.isRequired
+    walletAccount: nullableProptype(PropTypes.object.isRequired)


Possibly nullableProptype(PropTypes.object).isRequired (the full property is required, but could be either null or object)

jacogr · 2016-12-29T09:03:11Z

js/webpack/vendor.js

+
+  resolve: {
+    alias: {
+      '~': path.resolve(__dirname, '../src')


jacogr · 2016-12-29T10:57:33Z

js/src/ui/Form/AddressSelect/addressSelect.js

+      ? (hint || '')
+      : this.context.intl.formatMessage(
+        hint.props,
+        hint.props.values || []


hint.props.values || {}

It's totally valid for input's length not to be a multiple of 32 bytes. EG. for Wallet Contracts

GitCop · 2016-12-29T18:42:47Z

There were the following issues with your Pull Request

Commit: 234919d
Your commit message body contains a line that is longer than 72 characters

Guidelines are available at https://github.com/ethcore/parity

This message was auto-generated by https://gitcop.com

gavofyork · 2016-12-29T18:47:22Z

@ngotchac Yeah - we'll need an extensible architecture so that code hashes (i.e. types of wallet contracts) can be registered with specialised means of signing. This way dapps can use various addresses with eth_postTransaction and the signing UI will still work correctly - either passing it on to a derivative address (like for DeadMansSwitch) or multiple such addresses (the multisig wallet) or something else.

coveralls · 2016-12-29T19:31:29Z

Coverage decreased (-0.03%) to 86.566% when pulling 234919d on ng-wallet-improv into 3067a8d on master.

…wallet-improv

coveralls · 2016-12-29T21:02:14Z

Changes Unknown when pulling f087475 on ng-wallet-improv into ** on master**.

coveralls · 2016-12-29T21:42:51Z

Changes Unknown when pulling f087475 on ng-wallet-improv into ** on master**.

ngotchac added 16 commits December 27, 2016 15:34

Fixed hint in Address Select + Wallet as first-class-citizen

f5573df

Separate Owned and not Owned Wallets

aee74ce

Fix balance not updating

f3bf753

Merge branch 'master' into ng-wallet-improv

af23c58

Fix MethodDecoding for Contract Deployment

1b859bb

Fix TypedInput params

cb2bef6

Fix Token Transfer for Wallet

1234364

Small change to contracts

1c70fe7

Fix wallets shown twice

e0232ca

Fix separation of accounts and wallets in Accounts

03dd6b8

Fix linting

fe1e816

Execute contract methods from Wallet ✓

f371854

Fixing linting

a32bb86

Wallet as first-class citizen: Part 1 (Manual) #3784

17cb600

Lower level wallet transaction convertion

ea73917

Fix linting

c307fcc

ngotchac added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Dec 28, 2016

This was referenced Dec 28, 2016

Random Signer confirm/reject error pop-up #3570

Closed

JS error Transfer error ETH from one account to another from Parity UI #3849

Closed

jacogr reviewed Dec 29, 2016

View reviewed changes

js/webpack/vendor.js

resolve: {

alias: {

'~': path.resolve(__dirname, '../src')

Copy link

Contributor

jacogr Dec 29, 2016

Choose a reason for hiding this comment

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

+1

jacogr reviewed Dec 29, 2016

View reviewed changes

ngotchac added 2 commits December 29, 2016 17:40

Add postTransaction and gasEstimate wrapper methods

84f7b79

Extract Wallet postTx and gasEstimate to utils + PATCH api

69e484b

ngotchac added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Dec 29, 2016

gavofyork requested a review from derhuerst December 29, 2016 18:42

Remove invalid test

234919d

It's totally valid for input's length not to be a multiple of 32 bytes. EG. for Wallet Contracts

jacogr added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 29, 2016

ngotchac added 4 commits December 29, 2016 20:46

Merge branch 'master' into ng-wallet-improv

7a1acd7

Merge master

b191961

Fix linting

fb5d45b

Merge branch 'ng-wallet-improv' of github.com:ethcore/parity into ng-…

6895afe

…wallet-improv

ngotchac added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Dec 29, 2016

ngotchac added 3 commits December 29, 2016 20:54

Fix merge issue

1fdf8f6

Rename Portal

e4f782c

Rename Protal => Portal (typo)

f087475

ngotchac added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Dec 29, 2016

jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 30, 2016

gavofyork merged commit fd41a10 into master Dec 30, 2016

gavofyork deleted the ng-wallet-improv branch December 30, 2016 11:28

This was referenced Dec 30, 2016

When signer pops up, password should be focused #3923

Closed

UI: Wrong balance unit #4006

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Wallet first-class citizens #3990

Make Wallet first-class citizens #3990

ngotchac commented Dec 28, 2016 •

edited

Loading

gavofyork commented Dec 28, 2016

coveralls commented Dec 28, 2016

ngotchac commented Dec 28, 2016

jacogr Dec 29, 2016

jacogr Dec 29, 2016

jacogr Dec 29, 2016

jacogr Dec 29, 2016

jacogr Dec 29, 2016

jacogr Dec 29, 2016

jacogr Dec 29, 2016

GitCop commented Dec 29, 2016

gavofyork commented Dec 29, 2016 •

edited

Loading

coveralls commented Dec 29, 2016

coveralls commented Dec 29, 2016

coveralls commented Dec 29, 2016

Make Wallet first-class citizens #3990

Make Wallet first-class citizens #3990

Conversation

ngotchac commented Dec 28, 2016 • edited Loading

gavofyork commented Dec 28, 2016

coveralls commented Dec 28, 2016

ngotchac commented Dec 28, 2016

jacogr Dec 29, 2016

Choose a reason for hiding this comment

jacogr Dec 29, 2016

Choose a reason for hiding this comment

jacogr Dec 29, 2016

Choose a reason for hiding this comment

jacogr Dec 29, 2016

Choose a reason for hiding this comment

jacogr Dec 29, 2016

Choose a reason for hiding this comment

jacogr Dec 29, 2016

Choose a reason for hiding this comment

jacogr Dec 29, 2016

Choose a reason for hiding this comment

GitCop commented Dec 29, 2016

gavofyork commented Dec 29, 2016 • edited Loading

coveralls commented Dec 29, 2016

coveralls commented Dec 29, 2016

coveralls commented Dec 29, 2016

ngotchac commented Dec 28, 2016 •

edited

Loading

gavofyork commented Dec 29, 2016 •

edited

Loading