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

Supporting eth_sign in Signer #1787

Merged
merged 10 commits into from
Aug 3, 2016
Merged

Supporting eth_sign in Signer #1787

merged 10 commits into from
Aug 3, 2016

Conversation

tomusdrw
Copy link
Collaborator

@tomusdrw tomusdrw commented Aug 1, 2016

Closes #1419
Closes #1310

Yet to do:

  • UI changes
  • eth_postSign (?) - support async queries for sign (similar to eth_postTransaction)

@tomusdrw tomusdrw added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Aug 1, 2016
@tomusdrw tomusdrw changed the title Supporting eth_sign in Trusted Signer Supporting eth_sign in Signer Aug 1, 2016
@coveralls
Copy link

coveralls commented Aug 1, 2016

Coverage Status

Coverage decreased (-0.05%) to 86.816% when pulling b42aeab on signer-ethsign into f19b00b on master.

@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 Aug 2, 2016
@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage increased (+0.2%) to 87.042% when pulling 072074a on signer-ethsign into f19b00b on master.

@tomusdrw tomusdrw mentioned this pull request Aug 2, 2016
@gavofyork
Copy link
Contributor

gavofyork commented Aug 2, 2016

would have been nicer if the UI integration could have been reviewed in the same PR as the code behind it. instead we just get to "review" some anonymous hashes.

fn check_transaction(&self, _: Params) -> Result<Value, Error>;

/// Should be used to convert object to io delegate.
fn to_delegate(self) -> IoDelegate<Self> {
let mut delegate = IoDelegate::new(Arc::new(self));
delegate.add_method("eth_sign", EthSigning::sign);
delegate.add_method("eth_postSign", EthSigning::post_sign);
delegate.add_method("eth_sendTransaction", EthSigning::send_transaction);
delegate.add_method("eth_postTransaction", EthSigning::post_transaction);
delegate.add_method("eth_checkTransaction", EthSigning::check_transaction);
Copy link
Contributor

Choose a reason for hiding this comment

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

checkRequest instead of checkTransaction?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, I wasn't sure if it's ok to break backward compatibility (and I suppose one of your dapps was using this method).

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough (though another reason to have these parity/ethcore JS extensions in the same repo).

@gavofyork gavofyork 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 Aug 2, 2016
@gavofyork gavofyork merged commit 9fb5623 into master Aug 3, 2016
@gavofyork gavofyork deleted the signer-ethsign branch August 3, 2016 08:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants