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

Fix signing #4363

Merged
merged 4 commits into from
Jan 31, 2017
Merged

Fix signing #4363

merged 4 commits into from
Jan 31, 2017

Conversation

gavofyork
Copy link
Contributor

not sure if this is the right way to fix, but it works for me.

@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. M7-signer 🔏 Trusted signer. labels Jan 30, 2017
@gavofyork gavofyork requested a review from tomusdrw January 30, 2017 20:08
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Looks fine.

@@ -256,7 +256,7 @@ function mapStateToProps (_, initProps) {

return (state) => {
const { accounts } = state.personal;
let gotAddress = Object.keys(accounts).find(a => a.toLowerCase() === address);
let gotAddress = Object.keys(accounts).find(a => a.toLowerCase() === address.toLowerCase());
Copy link
Contributor

Choose a reason for hiding this comment

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

address.toLowerCase() could be memoized

Copy link
Contributor Author

@gavofyork gavofyork Jan 31, 2017

Choose a reason for hiding this comment

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

premature optimisation? this is a bug-fix PR.

@@ -120,7 +120,7 @@ export default class SignerMiddleware {
});
}

handlePromise(this._api.signer.confirmRequest(id, { gas, gasPrice }, password));
handlePromise(this._api.signer.confirmRequest(id, { gas, gasPrice }, password.password));
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 really working ? Would at least have typeof password === 'string' ? password : password.password or equivalent because it looks strange.

Copy link
Contributor Author

@gavofyork gavofyork Jan 31, 2017

Choose a reason for hiding this comment

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

the weirdness of naming an object with password and wallet fields password was already there. if this "looks strange", i suggest that variable be renamed rather than adding superfluous logic here.

however i don't know enough of the design behind this module to be sure if renaming is sensible. for sure the original version was buggy and this works under my current usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but the issue is comes from components passing different data types to callback props.
Actually, it's views/Signer/components/SignRequest/signRequest.js and views/Signer/components/TransactionPending/transactionPending.js that have different onConfirm method signature/argument types

Copy link
Contributor

Choose a reason for hiding this comment

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

views/Signer/components/TransactionPendingForm/TransactionPendingFormConfirm/transactionPendingFormConfirm.js is passing an Object the the onConfirm prop, and views/Signer/components/SignRequest/signRequest.js is expecting a string.

@ngotchac
Copy link
Contributor

I think that this should be the fix. @tomusdrw @gavofyork and @jacogr ?

diff --git a/js/src/redux/providers/signerMiddleware.js b/js/src/redux/providers/signerMiddleware.js
index 9e56602..b279459 100644
--- a/js/src/redux/providers/signerMiddleware.js
+++ b/js/src/redux/providers/signerMiddleware.js
@@ -120,7 +120,7 @@ export default class SignerMiddleware {
         });
     }
 
-    handlePromise(this._api.signer.confirmRequest(id, { gas, gasPrice }, password.password));
+    handlePromise(this._api.signer.confirmRequest(id, { gas, gasPrice }, password));
   }
 
   onRejectStart = (store, action) => {
diff --git a/js/src/views/Signer/components/SignRequest/signRequest.js b/js/src/views/Signer/components/SignRequest/signRequest.js
index 4a07453..c767697 100644
--- a/js/src/views/Signer/components/SignRequest/signRequest.js
+++ b/js/src/views/Signer/components/SignRequest/signRequest.js
@@ -152,8 +152,9 @@ export default class SignRequest extends Component {
     );
   }
 
-  onConfirm = password => {
+  onConfirm = (data) => {
     const { id } = this.props;
+    const { password } = data;
 
     this.props.onConfirm({ id, password });
   }

@ngotchac ngotchac added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 31, 2017
@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jan 31, 2017
@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 31, 2017
@gavofyork gavofyork merged commit 75ecabb into master Jan 31, 2017
@gavofyork gavofyork deleted the fix-signing branch January 31, 2017 13:15
jacogr pushed a commit that referenced this pull request Feb 3, 2017
gavofyork pushed a commit that referenced this pull request Feb 4, 2017
* s/Delete Contract/Forget Contract/ (#4237)

* Adjust the location of the signer snippet (#4155)

* Additional building-block UI components (#4239)

* Currency WIP

* Expand tests

* Pass className

* Add QrCode

* Export new components in ~/ui

* s/this.props.netSymbol/netSymbol/

* Fix import case

* ui/SectionList component (#4292)

* array chunking utility

* add SectionList component

* Add TODOs to indicate possible future work

* Add missing overlay style (as used in dapps at present)

* Add a Playground for the UI Components (#4301)

* Playground // WIP

* Linting

* Add Examples with code

* CSS Linting

* Linting

* Add Connected Currency Symbol

* 2015-2017

* 2015-2017

* 2015-2017

* 2015-2017

* 2015-2017

* 2015-2017

* 2015-2017

* Added `renderSymbol` tests

* PR grumbles

* Add Eth and Btc QRCode examples

* 2015-2017

* Add tests for playground

* Fixing tests

* Split Dapp icon into ui/DappIcon (#4308)

* Add QrCode & Copy to ShapeShift (#4322)

* Extract CopyIcon to ~/ui/Icons

* Add copy & QrCode address

* Default size 4

* Add bitcoin: link

* use protocol links applicable to coin exchanged

* Remove .only

* Display QrCode for accounts, addresses & contracts (#4329)

* Allow Portal to be used as top-level modal (#4338)

* Portal

* Allow Portal to be used in as both top-level and popover

* modal/popover variable naming

* export Portal in ~/ui

* Properly handle optional onKeyDown

* Add simple Playground Example

* Add proper event listener to Portal (#4359)

* Display AccountCard name via IdentityName (#4235)

* Fix signing (#4363)

* Dapp Account Selection & Defaults (#4355)

* Add parity_defaultAccount RPC (with subscription) (#4383)

* Default Account selector in Signer overlay (#4375)

* Typo, fixes #4271 (#4391)

* Fix ParityBar account selection overflows (#4405)

* Available Dapp selection alignment with Permissions (Portal) (#4374)

* registry dapp: make lookup use lower case (#4409)

* Dapps use defaultAccount instead of own selectors (#4386)

* Poll for defaultAccount to update dapp & overlay subscriptions (#4417)

* Poll for defaultAccount (Fixes #4413)

* Fix nextTimeout on catch

* Store timers

* Re-enable default updates on change detection

* Add block & timestamp conditions to Signer (#4411)

* Extension installation overlay (#4423)

* Extension installation overlay

* Pr gumbles

* Spelling

* Update Chrome URL

* Fix for non-included jsonrpc

* Extend Portal component (as per Modal) #4392
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. M7-signer 🔏 Trusted signer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants