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

Fix postsign #4347

Merged
merged 13 commits into from
Jan 30, 2017
Merged

Fix postsign #4347

merged 13 commits into from
Jan 30, 2017

Conversation

gavofyork
Copy link
Contributor

@gavofyork gavofyork commented Jan 29, 2017 •
edited
Loading

Yet to do:

  • fix tests;
  • fix "it thinks it's an client-side account even when it's not" bug;
  • detect ASCII data and provide link to render as text in popup window.

@gavofyork gavofyork added F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. M6-rpcapi 📣 RPC API. labels Jan 29, 2017
@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. and removed F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. labels Jan 30, 2017
tomusdrw
tomusdrw approved these changes Jan 30, 2017
View reviewed changes
Copy link
Collaborator

@tomusdrw tomusdrw left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Rust part looks good, although it may require some changes in the UI (but I guess those can follow, since there are two addtional issues to solve)

For minimal compatibility with the UI we should do sha3(sign.data) instead of sign.hash here:
https://github.com/ethcore/parity/blob/946d79f530f22982d3e83deaf8a95625c31bb15b/js/src/views/Signer/components/RequestPending/requestPending.js#L64

@tomusdrw tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 30, 2017
@gavofyork
Copy link
Contributor Author

@tomusdrw any idea about the "it thinks it's an client-side account even when it's not" bug?

@tomusdrw
Copy link
Collaborator

I've experienced something similar today when switching networks without reloading the GUI, maybe that was the case?
Will check if it works fine for me in a second.

@tomusdrw
Copy link
Collaborator

No, it's a different problem:

Here:
https://github.com/ethcore/parity/blob/f627bef08b5eb47e59c14ff6f0308c7b07e72c0e/js/src/views/Signer/components/TransactionPendingForm/TransactionPendingFormConfirm/transactionPendingFormConfirm.js#L259

address is lower cased, whereas accounts have checksum-cased addresses. Should be easy to fix.

@tomusdrw
Copy link
Collaborator

Logged as separate issues: #4352 #4353

@gavofyork
Copy link
Contributor Author

fixed both.

@gavofyork gavofyork mentioned this pull request Jan 30, 2017
Closed
@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Jan 30, 2017
@gavofyork
Copy link
Contributor Author

@tomusdrw ready for re-review.

tomusdrw
tomusdrw reviewed Jan 30, 2017
View reviewed changes
js/src/views/Signer/components/SignRequest/signRequest.css
@@ -27,8 +27,18 @@
min-height: $pendingHeight;
}

.signData {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be good to introduce maximal height and overflow for very long ascii texts:

overflow: auto;
max-height: 6em;

tomusdrw
tomusdrw approved these changes Jan 30, 2017
View reviewed changes
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 for me, minor grubmles

js/src/views/Signer/components/SignRequest/signRequest.js
@@ -62,8 +76,21 @@ export default class SignRequest extends Component {
);
}

renderAsciiDetails (ascii) {
return (<div className={ styles.signData }>
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's most common in the codebase to put a new line after ( and before ).

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 30, 2017
@gavofyork gavofyork merged commit ca196d6 into master Jan 30, 2017
@gavofyork gavofyork deleted the fix-postsign branch January 30, 2017 14:08
tomusdrw pushed a commit that referenced this pull request Feb 8, 2017
@gavofyork @tomusdrw
Fix postsign (#4347)
60654ee 
* Fix whitespace.

* Fix post sign.

* Fix message.

* Fix tests.

* Rest of the problems.

* All hail the linter and its omniscience.

* ...and its divine omniscience.

* Grumbles and wording.
@tomusdrw tomusdrw mentioned this pull request Feb 8, 2017
Merged
arkpar pushed a commit that referenced this pull request Feb 8, 2017
@tomusdrw @arkpar
eth_sign improvements backport (#4473)
8c049e5 
* Fix postsign (#4347)

* Fix whitespace.

* Fix post sign.

* Fix message.

* Fix tests.

* Rest of the problems.

* All hail the linter and its omniscience.

* ...and its divine omniscience.

* Grumbles and wording.

* Make signing compatible with geth. (#4468)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tomusdrw tomusdrw tomusdrw approved these changes

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. M6-rpcapi 📣 RPC API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@gavofyork @tomusdrw