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

Signer pt 1 #15702

Closed
wants to merge 4 commits into from
Closed

Signer pt 1 #15702

wants to merge 4 commits into from

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Dec 18, 2017

This is the first PR for the signer, containing some preparations for future work. Note for reviewers: This PR should not affect any existing feature or change any functionality in the way geth works today (so please shout if you believe it does). Contents of this PR:

  • toGoType -> ToGoType: to enable parsing calldata according to an ABI-specification.
  • Case-preserving address-type. This can (later on) be used instead of common.Address in e.g. to - field in sendTransaction, and will internally remember what the input was, instead of storing only the 20 bytes that make up the address. This will make it possible for geth to forward the data verbatim, and the UI can alert the user about erroneous checksums. Also, the signer API uses this already.
  • Stdio client: a new rpc client which uses stdin/stdout for communication
  • Audit log. This is useless in geth, which has credentials over the API, but is used in the signer which does not.
  • Forwarding of certain parameters, e.g. remote ip, so that the API methods can be aware of where the request comes from. If there are better ways of doing this, hints are appreciated. I'd like more values (e.g Origin), and don't like to hardcode every possible value into the http server.

@holiman holiman requested review from fjl and karalabe December 18, 2017 19:47
// into a go type with accordance with the ABI spec.
func toGoType(index int, t Type, output []byte) (interface{}, error) {
func ToGoType(index int, t Type, output []byte) (interface{}, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not happy about this change because you should be able to use

var v interface{}
abi.Unpack(&v, "method", calldata)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be glad for some suggestions on how to do it 'properly' -- I did it the only way I saw possible. See https://github.com/holiman/go-ethereum/blob/signer_mhs/cmd/signer/abihelper.go#L65 for more context.

I didn't know you could decode into an interface{}, I thought you had to allocate a suitable struct beforehand. I'll try your suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't quite work, the Unpack function aims at decoding returndata, not inputs.
It panics at e.g.

	marshalledValue, err := ToGoType(0, method.Outputs[0].Type, output)

Because I'm throwing it input, and have not even defined any output. A hacky way to go about it would be to retranslate inputs to outputs in my abi, so that instead of supplying

{"type":"function","name":"send","inputs":[{"type":"uint256"}]}

I would supply

{"type":"function","name":"send","outputs":[{"type":"uint256"}]}

But is that nice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible to make Unpack a bit more generic and handle inputs aswell as outputs... But I'm having some problems decoding 'tuple' types, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO we should make it work instead of exposing internals like toGoType.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Let's try to clean up in the abi-swamp

@holiman
Copy link
Contributor Author

holiman commented Dec 22, 2017

Closing, will reopen in after some rebase

@holiman holiman closed this Dec 22, 2017
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