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

Beamer check: add initiate-l1-invalidations command #2144

Merged
merged 5 commits into from
Aug 21, 2023
Merged

Conversation

istankovic
Copy link
Contributor

@istankovic istankovic commented Aug 17, 2023

This PR is part of #1646, adding the beamer check initiate-l1-invalidations command.

@istankovic istankovic force-pushed the beamer-check branch 2 times, most recently from a65f6ab to 0bba7ef Compare August 17, 2023 08:48
beamer/check/commands.py Outdated Show resolved Hide resolved
beamer/tests/util.py Outdated Show resolved Hide resolved
@istankovic istankovic marked this pull request as draft August 17, 2023 15:20
@istankovic istankovic force-pushed the beamer-check branch 3 times, most recently from 258fbc4 to 07a2a6b Compare August 17, 2023 16:11
@istankovic istankovic marked this pull request as ready for review August 17, 2023 16:12
@istankovic
Copy link
Contributor Author

Still missing; docs. 🤦

@fredo
Copy link
Contributor

fredo commented Aug 17, 2023

All in all, looks pretty good. I'd like to be aligned on my comment above though.

What's also missing (but part of the description in #1646) are the prove() calls for OP Stack chains.

@istankovic
Copy link
Contributor Author

What's also missing (but part of the description in #1646) are the prove() calls for OP Stack chains.

Yeah, I realized that just after marking this ready for review. 🤦
Will add, of course. (I'm not sure how to even approach testing this part so I'm probably going to leave it out of this PR. Maybe something based on top of our existing OP E2E tests, but even that does not seem fully straightforward...)

@fredo
Copy link
Contributor

fredo commented Aug 18, 2023

What's also missing (but part of the description in #1646) are the prove() calls for OP Stack chains.

Yeah, I realized that just after marking this ready for review. 🤦 Will add, of course. (I'm not sure how to even approach testing this part so I'm probably going to leave it out of this PR. Maybe something based on top of our existing OP E2E tests, but even that does not seem fully straightforward...)

I would be fine to push this into another PR cause we need to think a bit more about the information being passed here. For OP chains the finality period only starts after the prove() call was made, so we cannot really add the timestamp if the prove() didnt happen.

Alos here another concern is, that i.e. in the agent we try to call the prove() regardless of the state root being published and simply retry. But this also takes a couple of minutes. So we may want to even think about pushing the prove out of the invalidation creation?
Otherwise I can see that after every invalidation the script will halt until the state root has been published. We may want to consider flagging the invalidations which need to be proven and do this in a separate step or we need to properly handle the waiting time and the relayer exiting with an exception (if the state root is not published yet)

With regards to testing, in order to really test it we would need an actual OP setup like in the e2e tests...

@istankovic istankovic force-pushed the beamer-check branch 4 times, most recently from dfaa42c to 8f4ea9f Compare August 18, 2023 15:30
@istankovic
Copy link
Contributor Author

Alright, the docs are now also ready. 📜

beamer/check/commands.py Outdated Show resolved Hide resolved
beamer/check/commands.py Outdated Show resolved Hide resolved
docs/source/commands.rst Outdated Show resolved Hide resolved
docs/source/commands.rst Outdated Show resolved Hide resolved
@istankovic istankovic force-pushed the beamer-check branch 3 times, most recently from a3816fe to 9f63ac7 Compare August 21, 2023 13:47
Copy link
Contributor

@fredo fredo left a comment

Choose a reason for hiding this comment

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

very nice 🍷

@istankovic istankovic merged commit 1ecdfa0 into main Aug 21, 2023
1 check passed
@istankovic istankovic deleted the beamer-check branch August 21, 2023 15:42
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