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

Jump to bottom

[Merge] Execution engine channel implementation with client #4588

Merged

tbenr merged 11 commits into Consensys:master from tbenr:execution_engine_channel_implementation_with_client

Nov 15, 2021

Contributor

tbenr commented Nov 8, 2021 •

edited

Loading

PR Description

fixes #4593

Provide Execution engine channel implementation with Web3J calls to eth1_ namespace and direct JSON-RPC calls for engine_ namespace.

Documentation

I thought about documentation and added the documentation label to this PR if updates are required.

Changelog

I thought about adding a changelog entry, and added one if I deemed necessary.

ajsutton reviewed

View reviewed changes

.../main/java/tech/pegasys/teku/services/executionengine/client/schema/PayloadAttributesV1.java Show resolved Hide resolved

tbenr force-pushed the execution_engine_channel_implementation_with_client branch from 7746d62 to 67c06fc Compare

November 9, 2021 14:02

tbenr marked this pull request as ready for review

November 10, 2021 15:21

tbenr force-pushed the execution_engine_channel_implementation_with_client branch from 41e2fc2 to cd83058 Compare

November 10, 2021 16:29

tbenr commented

View reviewed changes

...ain/src/main/java/tech/pegasys/teku/services/executionengine/ExecutionEngineChannelImpl.java Outdated

+                public ExecutionEngineChannelImpl(ExecutionEngineClient executionEngineClient, Spec spec) {
+                  this.executionPayloadSchema =
+                      spec.forMilestone(SpecMilestone.MERGE)

Contributor Author

tbenr Nov 10, 2021 •

edited

Loading

do you think here is correct ATM? in the future should migrate to a dynamic atSlot and have a notion of the slot in getPayload

Contributor

ajsutton Nov 11, 2021

I think we might get away with this but it's definitely the wrong approach. I think I'd rather work out the details of getting the correct spec version now rather than build up tech debt that will have to be fixed before the next fork. The longer we leave it to do properly the more places that may depend on this magically working without knowing the relevant slot.

tbenr commented

View reviewed changes

...ain/src/main/java/tech/pegasys/teku/services/executionengine/ExecutionEngineChannelImpl.java Outdated Show resolved Hide resolved

ajsutton approved these changes

View reviewed changes

Contributor

ajsutton left a comment

LGTM. I think we can merge as-is but should follow up now to fix the schema definition handling and work out how we can look it up by slot correctly.

...eum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/execution/ExecutionPayload.java Outdated Show resolved Hide resolved

...ain/src/main/java/tech/pegasys/teku/services/executionengine/ExecutionEngineChannelImpl.java Outdated

+                public ExecutionEngineChannelImpl(ExecutionEngineClient executionEngineClient, Spec spec) {
+                  this.executionPayloadSchema =
+                      spec.forMilestone(SpecMilestone.MERGE)

Contributor

ajsutton Nov 11, 2021

I think we might get away with this but it's definitely the wrong approach. I think I'd rather work out the details of getting the correct spec version now rather than build up tech debt that will have to be fixed before the next fork. The longer we leave it to do properly the more places that may depend on this magically working without knowing the relevant slot.

...ain/src/main/java/tech/pegasys/teku/services/executionengine/ExecutionEngineChannelImpl.java Outdated Show resolved Hide resolved

...ain/src/main/java/tech/pegasys/teku/services/executionengine/ExecutionEngineChannelImpl.java Outdated Show resolved Hide resolved

...owchain/src/main/java/tech/pegasys/teku/services/executionengine/client/schema/Response.java Outdated Show resolved Hide resolved

tbenr and others added 6 commits

November 12, 2021 10:09


          implements ExecutionEngineChannel

fcf12c9


          improve client schema and implement UT

6143c09


          improve UT and implement actual calls with logging

cab65fc


          fix master rebase

implement additional logic in StubExecutionEngineChannel


          fix IT

4ea98d6


          runtime schema retrieval, change LOG level from debug to trace and cl…

4ed32e8

…eanup, toString() removed from ExecutionPayload ssz, Response holds more readable error field.

tbenr force-pushed the execution_engine_channel_implementation_with_client branch from d73a7b9 to 4ed32e8 Compare

November 12, 2021 10:35

tbenr commented

View reviewed changes

...ain/src/main/java/tech/pegasys/teku/services/executionengine/ExecutionEngineChannelImpl.java

+                }
+                @Override
+                public SafeFuture<ExecutionPayload> getPayload(final Bytes8 payloadId, final UInt64 slot) {

Contributor Author

tbenr Nov 12, 2021 •

edited

Loading

@ajsutton I changed the interface and the implementation here to lookup the schema at runtime

tbenr added 3 commits

November 12, 2021 12:14


          Merge branch 'master' into execution_engine_channel_implementation_wi…

25b2395

…th_client


          lambda logging

9ee7eaf


          spotlessly

267d66d

ajsutton reviewed

View reviewed changes

...src/testFixtures/java/tech/pegasys/teku/spec/executionengine/StubExecutionEngineChannel.java Outdated

Comment on lines 91 to 94

+                  if (executionPayloadSchema.isEmpty()) {
+                    return SafeFuture.failedFuture(
+                        new UnsupportedOperationException("getPayload not supported for non-Merge milestones"));
+                  }

Contributor

ajsutton Nov 14, 2021

Shouldn't we now use slot here to get the right schema from Spec?

Contributor Author

tbenr Nov 15, 2021

right, fixed

tbenr added 2 commits

November 15, 2021 09:07


          retrieve ExecutionPayload schema from spec in StubExecutionEngineChan…

6acc28f

…nel too


          Merge branch 'master' into execution_engine_channel_implementation_wi…

06f9e5b

…th_client

tbenr merged commit dcbe325 into Consensys:master

tbenr deleted the execution_engine_channel_implementation_with_client branch

November 15, 2021 09:11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet