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

Splitting serialization of signTransaction and sendTransaction confirmation requests #3642

Merged
merged 3 commits into from
Dec 1, 2016

Conversation

tomusdrw
Copy link
Collaborator

Closes #3560
Breaks compatibility with Chrome Plugin (we should probably mention in Release notes that it's already deprecated)

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. M7-ui labels Nov 28, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 85.928% when pulling 8686339 on confirmation into a7037f8 on master.

@tomusdrw tomusdrw added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Nov 28, 2016
@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 28, 2016
@gavofyork
Copy link
Contributor

lgtm. @jacogr / @ngotchac / @derhuerst could you cast a swift eye over the JS?

@@ -72,8 +72,8 @@ export default class SignerMiddleware {
};

// Sign request in-browser
if (wallet && payload.transaction) {
const { transaction } = payload;
if (wallet && (payload.sendTransaction || payload.signTransaction)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[tiny tiny grumble] Why not this?

const transaction = payload.sendTransaction || payload.signTransaction;
if (wallet && transaction) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point :) Fixing.

@@ -58,8 +59,8 @@ export default class RequestFinished extends Component {
);
}

if (payload.transaction) {
const { transaction } = payload;
if (payload.sendTransaction || payload.signTransaction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

@tomusdrw tomusdrw added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Nov 28, 2016
@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Nov 29, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 85.953% when pulling 1a6ee53 on confirmation into a7037f8 on master.

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 29, 2016
@gavofyork gavofyork merged commit c630fc5 into master Dec 1, 2016
@gavofyork gavofyork deleted the confirmation branch December 1, 2016 01:09
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. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants