-
Notifications
You must be signed in to change notification settings - Fork 5
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: aggsender e2e #183
feat: aggsender e2e #183
Conversation
76e10de
to
dc75fe3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking just to not forgot and merge with "jesteban/bump_aggsender"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Folder naming is inconsistent: fep
vs pessimistic
it should be pp
+ fep
or pessimistic
+ full-execution
or another option but consistent
Aside from that, to really be an e2e test, it should work the same way a user would do, querying the DB is something not realistic for the user. To check the certificates we should query the AggLayer using getCertificateHeader
RPC
or getLatestKnownCertificateHeader
that should be enough to know that the certificate was sent correctly.
Ok, I wil change pessimistic to pp
The test, as I explained, is a minium version and not ideal, for sure. The end-point required to retrieve a certificate is not released yet ( check issue) , so the best approach is check on DB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test, as I explained, is a minium version and not ideal, for sure. The end-point required to retrieve a certificate is not released yet ( check issue) , so the best approach is check on DB
I understand the situation, without those endpoints I agree checking the DB is the best way to check for certs, but the endpoints are released in 0.2.0-RC as I checked with the team, it should work with the image in this PR, the issue was closed a few mins ago, so let's not introduce more short-time debt here and work with what the agglayer have, no?
We have another PR to integrating this new end-point, so it make sense to merge this one as-is and in the new PR update the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with this until we it's refactored in #192
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM
Quality Gate passedIssues Measures |
Description
aggsender
database if there are 1 settle certificate