-
Notifications
You must be signed in to change notification settings - Fork 463
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
Feature external signer #6780
Feature external signer #6780
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wny not implement this as a plugin instead. does not feel like it should be part of the main client
Good idea - i will refactor it into a plugin instead. |
commit ab6f5be Author: ak88 <anders.holmbjerg@hotmail.com> Date: Mon Mar 25 13:36:30 2024 +0100 unittest commit ad2bf4c Author: ak88 <anders.holmbjerg@hotmail.com> Date: Mon Mar 25 11:49:35 2024 +0100 small fixes commit 503db49 Author: ak88 <anders.holmbjerg@hotmail.com> Date: Thu Mar 21 15:17:54 2024 +0100 plugin methods commit 1034179 Author: ak88 <anders.holmbjerg@hotmail.com> Date: Thu Mar 21 12:57:53 2024 +0100 clef signer as plugin
src/Nethermind/Nethermind.JsonRpc.Connectors/Nethermind.JsonRpc.Clients.csproj
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.JsonRpc.Connectors/Client/BasicJsonRpcClient.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.JsonRpc.Connectors/Client/BasicJsonRpcClient.cs
Outdated
Show resolved
Hide resolved
@@ -70,4 +70,6 @@ public string ExtraData | |||
return _blocksConfig; | |||
} | |||
} | |||
|
|||
public string Signer { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is not possible, but could we make it an Uri?
type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not possible with the current JsonSerializer, but a custom one could be made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should write one, is easy - here with JSON.NET: https://stackoverflow.com/a/8087049/1187056
…pcClient.cs Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com>
IHeaderSigner signer = Substitute.For<IHeaderSigner>(); | ||
signer.CanSignHeader.Returns(true); | ||
signer.CanSign.Returns(true); | ||
signer.Address.Returns(new Address("0x7ffc57839b00206d1ad20c69a1981b489f772031")); | ||
signer.Sign(Arg.Any<BlockHeader>()).Returns(new Signature(new byte[65])); | ||
CliqueSealer sut = new CliqueSealer(signer, new CliqueConfig(), _snapshotManager, LimboLogs.Instance); | ||
Block block = Rlp.Decode<Block>(new Rlp(Bytes.FromHexString(blockRlp))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated code
BlockHeader clone = header.Clone(); | ||
clone.ExtraData = SnapshotManager.SliceExtraSealFromExtraData(clone.ExtraData); | ||
clone.Hash = headerHash; | ||
signature = headerSigner.Sign(clone); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we hide it somewhere in IHeaderSigner
or with some extension method?
IJsonRpcClient client = Substitute.For<IJsonRpcClient>(); | ||
client.Post<string[]>("account_list").Returns(Task.FromResult<string[]?>([TestItem.AddressA!.ToString()])); | ||
Task<string?> postMethod = client.Post<string>("account_signData", "text/plain", Arg.Any<string>(), Keccak.Zero); | ||
var returnValue = (new byte[65]).ToHexString(); | ||
postMethod.Returns(returnValue); | ||
ClefSigner sut = await ClefSigner.Create(client); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly a lot of duplicate in setup
@ak88 Hi, I see that the src/Nethermind/Nethermind.ExternalSigner.Plugin/ClefSignerPlugin.cs |
@anhnhgutech that is a good catch, would you care to try creating PR that handles reconnections? |
Adds clef as a remote signer.
https://geth.ethereum.org/docs/tools/clef/introduction
Changes
ClefSigner
that can communicate using JsonRpcCliqueSealer
will seal with remote signer if available--Mining.Signer
for setting url of the remote signerKeyStore.BlockAuthorAccount
will attempt to sign with that account if available on remoteWhat types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Documentation
Configuration of clef and using the correct flags needs to be documented.
https://geth.ethereum.org/docs/tools/clef/clique-signing#prepping-clef
Requires documentation update