clef: make external signing work + follow up fixes #19003
Merged
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
…ounts_sign take hexdata
…ue signing in clef
|
Output on clef-side when running the script |
gballet
requested changes
Feb 11, 2019
cmd/clef/intapi_changelog.md
Outdated
| @@ -1,8 +1,15 @@ | |||
| ### Changelog for internal API (ui-api) | |||
|
|
|||
| ### 3.2.0 | |||
|
|
|||
| * Make `ShowError`, `OnApprovedTx`, `OnSignerStartup` be json-rpc [notification](https://www.jsonrpc.org/specification#notification): | |||
There was a problem hiding this comment.
Make ShowError, OnApprovedTx, AND OnSignerStartup (remove be) json-rpc [notificationS]
| @@ -1481,6 +1482,42 @@ func (api *PublicDebugAPI) GetBlockRlp(ctx context.Context, number uint64) (stri | |||
| return fmt.Sprintf("%x", encoded), nil | |||
| } | |||
|
|
|||
| // TestSignCliqueBlock fetches the given block number, and attempts to sign it as a clique header with the | |||
| // given address, returning the address of the recovered signature | |||
There was a problem hiding this comment.
Reminder: Peter said that this should be noted as TODO/to remove
signer/core/signed_data.go
Outdated
| func (api *SignerAPI) determineSignatureFormat(ctx context.Context, contentType string, addr common.MixedcaseAddress, data interface{}) (*SignDataRequest, bool, error) { | ||
| var ( | ||
| req *SignDataRequest | ||
| useLegacyV = true // Default to use V = 27 or 28, the legacy Ethereum format |
There was a problem hiding this comment.
As per our conversation, let's use useEthereumV or useCliqueV as a name instead.
| var tests = [ | ||
| testTx, | ||
| testSignText, | ||
| testClique, |
There was a problem hiding this comment.
could we also have tests to check the error conditions?
There was a problem hiding this comment.
Are there any particular error conditions you're thinking of?
I mean, there are quite a lot of ways for signing to fail. This js-test is meant as an integration test, to test the whole geth-external-ui and back again flow.
It's not for CI, but for a dev to run manually to test things more easily.
Individual tests within signer/clef should test things like rejection and tx validation etc.
There was a problem hiding this comment.
I'm thinking of the 27/28 vs 0/1 in particular.
There was a problem hiding this comment.
Ok, added for transactions. For clique, the new debug endpoint will deliver the derived signer. And if the wrong v-type is used, it will derive another address as sealer of that block.
So the js-test already checks that the requested signer-address equals what the endpoint returns, thus checks that the geth and clef are in agreement (without explicitly checking v)
kiku-jw
pushed a commit
to kiku-jw/go-ethereum
that referenced
this pull request
Mar 29, 2019
* signer/clef: make use of json-rpc notification * signer: tidy up output of OnApprovedTx * accounts/external, signer: implement remote signing of text, make accounts_sign take hexdata * clef: added basic testscript * signer, external, api: add clique signing test to debug rpc, fix clique signing in clef * signer: fix clique interoperability between geth and clef * clef: rename networkid switch to chainid * clef: enable chainid flag * clef, signer: minor changes from review * clef: more tests for signer
This was referenced May 6, 2022
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.
This is a follow-up PR to fix issues after the merging of external-signer, bidirectional communication and implementation of eip 712/192.
It contains
%q)--networkidinto--chainidOne thing still remaining is to display remote and local address for ipc communication, but that can be left for a later PR.