ethclient, mobile: add TransactionSender #15127
Merged
+204
−149
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
d3846ea to
68de1c3
Compare
There are two reasons to do this now: The upcoming ethclient signer doesn't know the public key, just the address. EIP 208 will introduce a new signer which derives the 'entry point' address for transactions with zero signature. The entry point has no public key. Other changes to the interface ease the path make to moving signature crypto out of core/types later.
984140b to
3e6bf9c
Compare
The new method can get the right signer without any crypto, and without knowledge of the signature scheme that was used when the transaction was included.
3e6bf9c to
e4ae593
Compare
bas-vk
reviewed
Sep 18, 2017
ethclient/signer.go
Outdated
|
|
||
| var errNotCached = errors.New("sender not cached") | ||
|
|
||
| func setSenderFromServer(tx *types.Transaction, addr common.Address) { |
There was a problem hiding this comment.
We can call this method also from getBlock since the response always contains full transaction details. That saves a round up trip to the node when the client calls TransactionSender on the transactions in the block.
fjl
commented
Sep 20, 2017
ethclient/ethclient.go
Outdated
| // TODO It'd better to use GetTransactionByBlockHashAndIndex because it works with the | ||
| // light client. This will be even more important with EIP 208 because there can be | ||
| // multiple inclusions of the same tx. The downside is that users would need to supply | ||
| // the inclusion block. |
There was a problem hiding this comment.
@bas-vk I need your feedback on this comment because it would change the method signature. I don't want to change the signature later.
|
@bas-vk PTAL |
bas-vk
reviewed
Sep 27, 2017
mobile/ethclient.go
Outdated
| addr, err := ec.client.TransactionSender(ctx.context, tx.tx) | ||
| // GetTransactionSender returns the sender address of a transaction. The transaction must | ||
| // be included in the blockchain. | ||
| func (ec *EthereumClient) GetTransactionSender(ctx *Context, tx *Transaction, hash *Hash, index int) (sender *Address, _ error) { |
There was a problem hiding this comment.
Please document that hash and index points to a block hash and tx index. That is not obvious.
|
@bas-vk PTAL |
|
👍 |
vincentserpoul
pushed a commit
to vincentserpoul/go-ethereum
that referenced
this pull request
Nov 22, 2017
* core/types: make Signer derive address instead of public key There are two reasons to do this now: The upcoming ethclient signer doesn't know the public key, just the address. EIP 208 will introduce a new signer which derives the 'entry point' address for transactions with zero signature. The entry point has no public key. Other changes to the interface ease the path make to moving signature crypto out of core/types later. * ethclient, mobile: add TransactionSender The new method can get the right signer without any crypto, and without knowledge of the signature scheme that was used when the transaction was included.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The new method can get the right signer without any crypto, and without
knowledge of the signature scheme that was used when the transaction was
included.