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.

Sign up for GitHub

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

Implement engine_exchangeCapabilities method #5212

Merged
merged 8 commits into from
Jan 27, 2023
Merged

Implement engine_exchangeCapabilities method #5212

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c20eaa7
Implement `engine_getCapabilities` method
rubo Jan 25, 2023
f0da7bc
Update `engine_exchangeCapabilities` implementation
rubo Jan 26, 2023
d0e0f01
Add capabilities warning test
rubo Jan 26, 2023
23ec12d
Extract capabilities handling
rubo Jan 26, 2023
e70fcd8
Revise the method name
rubo Jan 27, 2023
a57e303
Revise warning logic
rubo Jan 27, 2023
78ea320
Improve warning test
rubo Jan 27, 2023
dcc4d73
Update capabilities order
rubo Jan 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.V1.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1661,6 +1661,22 @@ await rpc.engine_forkchoiceUpdatedV1(forkChoiceState1,
}
}

[Test]
public async Task Should_return_capabilities()
{
using var chain = await CreateBlockChain();
var rpcModule = CreateEngineModule(chain);

var expected = typeof(IEngineRpcModule).GetMethods()
Copy link
Contributor

Choose a reason for hiding this comment

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

your tests are the same as the code, so we're not testing anything IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How's that? If you change something in the method test will catch it. The fact they are the same doesn't mean they are connected or affect each other.

Copy link
Member

Choose a reason for hiding this comment

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

you may introduce the same bug in both, test should hardcode your expectations and should read like a spec

.Select(m => m.Name)
.Where(m => !m.Equals(nameof(IEngineRpcModule.engine_getCapabilities), StringComparison.Ordinal))
.Order();

var result = rpcModule.engine_getCapabilities();

result.Data.Should().BeEquivalentTo(expected);
}

private async Task<ExecutionPayload> BuildAndGetPayloadResult(
IEngineRpcModule rpc, MergeTestBlockchain chain, Keccak headBlockHash, Keccak finalizedBlockHash,
Keccak safeBlockHash,
Expand Down
13 changes: 13 additions & 0 deletions src/Nethermind/Nethermind.Merge.Plugin/EngineRpcModule.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// SPDX-License-Identifier: LGPL-3.0-only

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Nethermind.Core.Crypto;
using Nethermind.Core.Specs;
Expand All @@ -14,6 +16,7 @@ namespace Nethermind.Merge.Plugin;

public partial class EngineRpcModule : IEngineRpcModule
{
private static IEnumerable<string>? _capabilities;
private readonly IHandler<ExecutionStatusResult> _executionStatusHandler;
private readonly IAsyncHandler<Keccak[], ExecutionPayloadBodyV1Result?[]> _executionGetPayloadBodiesByHashV1Handler;
private readonly IGetPayloadBodiesByRangeV1Handler _executionGetPayloadBodiesByRangeV1Handler;
Expand Down Expand Up @@ -46,6 +49,16 @@ public EngineRpcModule(

public ResultWrapper<ExecutionStatusResult> engine_executionStatus() => _executionStatusHandler.Handle();

public ResultWrapper<IEnumerable<string>> engine_getCapabilities()
{
_capabilities ??= typeof(IEngineRpcModule).GetMethods()
MarekM25 marked this conversation as resolved.
Show resolved Hide resolved
.Select(m => m.Name)
.Where(m => !m.Equals(nameof(engine_getCapabilities), StringComparison.Ordinal))
.Order();

return ResultWrapper<IEnumerable<string>>.Success(_capabilities);
}

public async Task<ResultWrapper<ExecutionPayloadBodyV1Result?[]>> engine_getPayloadBodiesByHashV1(Keccak[] blockHashes)
{
return await _executionGetPayloadBodiesByHashV1Handler.HandleAsync(blockHashes);
Expand Down
7 changes: 7 additions & 0 deletions src/Nethermind/Nethermind.Merge.Plugin/IEngineRpcModule.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// SPDX-FileCopyrightText: 2022 Demerzel Solutions Limited
// SPDX-License-Identifier: LGPL-3.0-only

using System.Collections.Generic;
using System.Threading.Tasks;
using Nethermind.Core.Crypto;
using Nethermind.JsonRpc;
Expand All @@ -19,6 +20,12 @@ public partial interface IEngineRpcModule : IRpcModule
IsImplemented = true)]
ResultWrapper<ExecutionStatusResult> engine_executionStatus();

[JsonRpcMethod(
Description = "Returns the currently supported list of Engine API methods.",
IsSharable = true,
IsImplemented = true)]
ResultWrapper<IEnumerable<string>> engine_getCapabilities();
Copy link
Contributor

Choose a reason for hiding this comment

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

let's confirm the method name


[JsonRpcMethod(
Description = "Returns an array of execution payload bodies for the list of provided block hashes.",
IsSharable = true,
Expand Down