Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom signing methods #287

Merged
merged 4 commits into from
Nov 21, 2019
Merged

Conversation

reuvenpo
Copy link
Contributor

This is a follow up PR to the ideas in this comment on #279.

I define equivalents of methods that call functions on secret contracts, but i sign the transactions locally instead of letting the ethereum node sign them for me with an unlocked account.

The first commit addresses the issue pointed out in this comment (Signing should accept arbitrary binary data rather than only UTF-8 data.)

…data.

Previously, it could only sign std::String and std::str (UTF-8) data.
This reflects the functionality in the Go implementation of message signing:
https://github.com/bas-vk/go-ethereum/blob/e61035c5a3630e4f6fd0fb3e5346a4eed8cedc80/internal/ethapi/api.go#L352
as well as what is available in the `ethereum-tx-sign` crate:
https://docs.rs/ethereum-tx-sign/2.0.0/ethereum_tx_sign/struct.RawTransaction.html
(note the `data` field is a Vec<u8>)
Copy link
Owner

@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.

Perfect! 👌

@tomusdrw
Copy link
Owner

Can you fix the formatting issue?

@reuvenpo
Copy link
Contributor Author

I fixed the formatting.
Please note that I opened this PR to implement the methods on Contract that my team needed, and i wrote these first few commits to build up to this, but this morning we de-prioritized this task and will sadly not complete it :/
Feel free to either merge this directly, or continue the work this PR was started for.


My only note would be that the fields of web3::types::signed::{SignedData, SignedTransaction} should be set to pub(crate) and methods on those types must be implemented to expose the values of these fields as readonly. It does not currently make sense that a user can have an instance of these types, and then write to it. That would break the invariant this type supposedly provides. I was gonna do this myself but i lost myself in a rabbit hole trying to understand the s, r, v fields. It should be documented which exact form of the v field is used in each one of the structs here, and perhaps even provide wrapper types for different v formats, along with From implementations between them. Again, it's hard to understand from the code as it is, what values can be expected in this field, and if it's even consistent between instances of the type.

@tomusdrw
Copy link
Owner

Thanks for working on this! I feel that we don't really give any guarantees about the types, you can still get the error when you try to recover a signature from them, and being able to construct them outside is probably useful too, so I'll keep it as is.

@tomusdrw tomusdrw merged commit 355f092 into tomusdrw:master Nov 21, 2019
@reuvenpo
Copy link
Contributor Author

what do you mean by "the error"?

@tomusdrw
Copy link
Owner

`@reuvenpo This error for instance:

let public_key = signature.recover(&message_hash[..]).map_err(EthsignError::from)?;

@reuvenpo
Copy link
Contributor Author

Ah, i see your point. Very well 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants