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

op-node: Move withdrawal utils out of node into op-e2e #9771

Closed
wants to merge 1 commit into from

Conversation

ajsutton
Copy link
Contributor

@ajsutton ajsutton commented Mar 7, 2024

Description

op-node: Move withdrawal utils out of node into op-e2e.

These are intended to be utilities for e2e tests so shouldn't live with the production code in op-node.

Additional context

This likely still creates a problem for Base's withdrawer tool - see #9765 Keeping as a draft until we settle on a plan but figured I'd put this up so CI can run tests and see if it does break anything.

If we intend to make these withdrawal utilities supported we probably should put them in a top level package or move them to op-service (and clean them up and test them a lot better). They don't actually get used by op-node so doesn't make sense to keep them there.

These are intended to be utilities for e2e tests so shouldn't live with the production code in op-node.
@ajsutton ajsutton force-pushed the aj/move-withdrawals branch from 943fc1b to 4d35b73 Compare March 7, 2024 05:11
@mdehoog
Copy link
Contributor

mdehoog commented Mar 7, 2024

@ajsutton thanks for the review and merge of #9765.

I understand the desire to move this package to the op-e2e group. If that happened, we would probably extract this and maintain our own. However I would like to put a vote in for keeping this where it is, simply because it's nice to have a Golang implementation of withdrawal logic. I would argue that just because it's only used in E2E testing, doesn't mean it doesn't serve a purpose as a separate Golang package that can be used by other Golang tools for withdrawals.

For example, we have Golang tools that perform automated withdrawals from our vaults into our batcher/proposer addresses, which depend on this logic.

I like your idea of moving to op-service 👍

@ajsutton
Copy link
Contributor Author

ajsutton commented Mar 7, 2024

Yeah I agree it can be a useful library. Any chance you'd have time to move it to op-service and maybe tidy it up a little and see if we can improve testing? Doesn't have to be perfect but at the moment I think it still "looks" like e2e utils so people don't realise they shouldn't introduce e2e dependencies.

I don't actually know what I'd change yet - I could have a play but open to any ideas.

@tynes
Copy link
Contributor

tynes commented Mar 7, 2024

The same way that geth has abi packages, op-node could have withdrawal packages. But we do have a monorepo so i could see the argument for moving it out of op-node proper. We need an official sdk (in any language) so that we can deprecate the ts sdk completely and have people use viem when using typescript. viem team is only comfy with maintaining the only ts sdk if there is at least another sdk in any language

@ajsutton
Copy link
Contributor Author

I do think op-service is probably the right place for this but don't have time to try and push this forward at the moment. I suspect that currently the code looks enough like a test utility that we are very likely to re-introduce issues like depending on op-e2e again in the future. The impact of that was pretty low though so if it happens again we can look further into how to fix it better.

@ajsutton ajsutton closed this Mar 17, 2024
@ajsutton ajsutton deleted the aj/move-withdrawals branch October 23, 2024 22:33
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