Skip to content

Signer pt 1#15702

Closed
holiman wants to merge 4 commits into
ethereum:masterfrom
holiman:signer_part_one
Closed

Signer pt 1#15702
holiman wants to merge 4 commits into
ethereum:masterfrom
holiman:signer_part_one

Conversation

@holiman
Copy link
Copy Markdown
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
Comment thread accounts/abi/unpack.go
// ToGoType parses the output bytes and recursively assigns the value of these bytes
// 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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