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.

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

feat: use v4 engine APIs when Isthmus enabled #13976

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

meyer9
Copy link
Contributor

@meyer9 meyer9 commented Jan 24, 2025

Description

Updates Optimism to use payload v4 methods when Isthmus is enabled.

Tests

Updates existing tests following v3 example.

@tynes
Copy link
Contributor

tynes commented Jan 28, 2025

/ci authorize 871efb6

@tynes
Copy link
Contributor

tynes commented Jan 28, 2025

@meyer9 what are your plans with this PR?

@meyer9
Copy link
Contributor Author

meyer9 commented Jan 28, 2025

Hmm it needs either #13933 or #13973 to pass CI

I see the second one is closed, so I reopened the first one.

@meyer9
Copy link
Contributor Author

meyer9 commented Jan 28, 2025

oh nvm looks like the relevant changes were already merged

@meyer9 meyer9 force-pushed the meyer9/call-new-payload-v4 branch from 871efb6 to 3a3de57 Compare January 28, 2025 15:54
@meyer9 meyer9 marked this pull request as ready for review January 28, 2025 15:54
@meyer9 meyer9 requested review from a team as code owners January 28, 2025 15:54
@meyer9 meyer9 requested review from refcell and protolambda January 28, 2025 15:54
@tynes
Copy link
Contributor

tynes commented Jan 29, 2025

/ci authorize 3a3de57

req[i] = hex[i]
}
return req
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if its possible to do this in a 1 liner but doesn't seem possible without unsafe

// Warning: not recommended for production.
func convertRequestsUnsafe(hex []hexutil.Bytes) [][]byte {
	return *(*[][]byte)(unsafe.Pointer(&hex))
}

@@ -352,6 +357,36 @@ func (ea *L2EngineAPI) NewPayloadV3(ctx context.Context, params *eth.ExecutionPa
return ea.newPayload(ctx, params, versionedHashes, beaconRoot, nil)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks right to me but not sure if there is something else missing in here. cc @Inphi

return &eth.PayloadStatusV1{Status: eth.ExecutionInvalid}, engine.InvalidParams.With(errors.New("nil executionRequests post-prague"))
}

if !ea.config().IsIsthmus(uint64(params.Timestamp)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't notice test coverage of this case

@tynes
Copy link
Contributor

tynes commented Jan 29, 2025

/ci authorize 3a3de57

@tynes
Copy link
Contributor

tynes commented Jan 30, 2025

Looks like there was some sort of deadlock?

=== FAIL: op-e2e/interop  (0.00s)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants