Skip to content

op-e2e: Adopt op-chain-ops#3649

Merged
mergify[bot] merged 13 commits intodevelopfrom
10-04-op-e2e_Start_adopting_op-chain-ops
Oct 4, 2022
Merged

op-e2e: Adopt op-chain-ops#3649
mergify[bot] merged 13 commits intodevelopfrom
10-04-op-e2e_Start_adopting_op-chain-ops

Conversation

@mslipper
Copy link
Contributor

@mslipper mslipper commented Oct 4, 2022

This PR updates op-e2e to use op-chain-ops. Adopting op-chain-ops in op-e2e improves the following:

  1. Transactions now route through the correct proxy contracts.
  2. There's no need to wait for a contract deployment since conracts exist in genesis.
  3. We can export the test helpers in op-e2e to enable us to run similar end-to-end tests in other applications like the indexer

This PR starts the process of adopting op-chain-ops in op-e2e by porting over the `Secrets` struct we use in Hive. This lets us share wallet generation code among different projects.

Adopting op-chain-ops in op-e2e improves the following:

1. Transactions now route through the correct proxy contracts.
2. There's no need to wait for a contract deployment since conracts exist in genesis.
3. We can export the test helpers in op-e2e to enable us to run similar end-to-end tests in other applications like the indexer

This is the first in a series of stacked PRs.
@changeset-bot
Copy link

changeset-bot bot commented Oct 4, 2022

⚠️ No Changeset found

Latest commit: caf72b7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mslipper
Copy link
Contributor Author

mslipper commented Oct 4, 2022

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@mergify
Copy link
Contributor

mergify bot commented Oct 4, 2022

This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?

@trianglesphere
Copy link
Contributor

@mslipper I think the secrets have already been ported over to the e2eutils folder: https://github.com/ethereum-optimism/optimism/blob/develop/op-e2e/e2eutils/secrets.go

@mslipper
Copy link
Contributor Author

mslipper commented Oct 4, 2022

I rolled #3650 in here since the secrets util was already included - will push once tests pass locally.

@mslipper mslipper changed the title op-e2e: Start adopting op-chain-ops op-e2e: Adopt op-chain-ops Oct 4, 2022
@mslipper
Copy link
Contributor Author

mslipper commented Oct 4, 2022

Removing dead code made this a hair over PR size SLA.

@tynes
Copy link
Contributor

tynes commented Oct 4, 2022

being able to adopt this code and have it used as the test infra is nice, gives more confidence in using it for the actual state surgery

Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

2 small things, but I'm excited for this

mslipper and others added 2 commits October 4, 2022 13:10
Co-authored-by: Joshua Gutow <jgutow@optimism.io>
Co-authored-by: Joshua Gutow <jgutow@optimism.io>
@mslipper mslipper closed this Oct 4, 2022
@mslipper mslipper reopened this Oct 4, 2022
Co-authored-by: Joshua Gutow <jgutow@optimism.io>
@tynes
Copy link
Contributor

tynes commented Oct 4, 2022

I'd like to get this merged so I can use it to write better tests for #3528
Realizing that the tests that I'd like to write belong more in op-e2e than in op-chain-ops

@mslipper
Copy link
Contributor Author

mslipper commented Oct 4, 2022

I'm debugging a test timeout. Stand by.

@mslipper
Copy link
Contributor Author

mslipper commented Oct 4, 2022

Ok, this one was really interesting - the timeout was caused by a deadlock in the SyncStatus function. Here's the implementation:

func (s *state) SyncStatus(ctx context.Context) (*eth.SyncStatus, error) {
	respCh := make(chan eth.SyncStatus)
	select {
	case <-ctx.Done():
		return nil, ctx.Err()
	case s.syncStatusReq <- respCh:
		select {
		case <-ctx.Done():
			return nil, ctx.Err()
		case resp := <-respCh:
			return &resp, nil
		}
	}
}

In some cases, the e2e test would cancel the passed-in context. This caused the function to return. If the timing as correct, the context would be canceled before the driver loop had a chance to send a response to respCh. This caused closing the rollup node to hang, since the listener for the done channel in Close() is inside the big driver loop.

The solution was to make respCh buffered, so that the driver can still send responses to the channel after the SyncStatus context is closed.

@mergify
Copy link
Contributor

mergify bot commented Oct 4, 2022

This PR has been added to the merge queue, and will be merged soon.

@mergify mergify bot merged commit f7a591c into develop Oct 4, 2022
@mergify mergify bot deleted the 10-04-op-e2e_Start_adopting_op-chain-ops branch October 4, 2022 23:28
@mergify
Copy link
Contributor

mergify bot commented Oct 4, 2022

This PR is next in line to be merged, and will be merged as soon as checks pass.

@mergify mergify bot removed the on-merge-train label Oct 4, 2022
This was referenced Oct 13, 2022
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.

3 participants