Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: aggsender e2e #183

Merged
merged 14 commits into from
Nov 19, 2024
Merged

feat: aggsender e2e #183

merged 14 commits into from
Nov 19, 2024

Conversation

joanestebanr
Copy link
Contributor

@joanestebanr joanestebanr commented Nov 13, 2024

Description

  • Split FEP and PP tests
  • Add PP that check on aggsender database if there are 1 settle certificate
  • Add sqlite client to docker

Copy link
Contributor

@vcastellm vcastellm left a 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"

Copy link
Contributor

@vcastellm vcastellm left a 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.

@joanestebanr
Copy link
Contributor Author

joanestebanr commented Nov 15, 2024

Folder naming is inconsistent: fep vs pessimistic it should be pp + fep or pessimistic + full-execution or another option but consistent

Ok, I wil change pessimistic to pp

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.

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

Copy link
Contributor

@vcastellm vcastellm left a 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?

@joanestebanr
Copy link
Contributor Author

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 aggsender code and also the e2e test to take advantatge of this

Copy link
Contributor

@vcastellm vcastellm left a 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

Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal left a comment

Choose a reason for hiding this comment

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

Generally LGTM

@joanestebanr joanestebanr merged commit df57e65 into develop Nov 19, 2024
11 checks passed
@joanestebanr joanestebanr deleted the feature/aggsender-e2e branch November 19, 2024 14:40
Vui-Chee added a commit to okx/xlayer-cdk that referenced this pull request Dec 3, 2024
* tag 'v0.5.0-beta6':
  feat: write on database the number of retries per certificate and the certificates in a history table (0xPolygon#208)
  feat: return an error in case agglayer returns certificate with height lower than in local storage (0xPolygon#209)
  chore: simplify the `HashMeddler` (0xPolygon#205)
  fix: clean proof table on start (0xPolygon#207) (0xPolygon#210)
  feat: improve logs (0xPolygon#204)
  fix: cdk603 error calculating previousLocalExitRoot (0xPolygon#199)
  fix: Integration Bali PP (0xPolygon#198)
  feat: check agglayer certificate and use as initial if db is empty (0xPolygon#192)
  feat: sqlite aggregator (0xPolygon#189)
  feat: BridgeMessage e2e test (0xPolygon#184)
  feat: aggsender e2e (0xPolygon#183)
  fix: aggregating proofs (0xPolygon#191) (0xPolygon#193)
  feat: l1infotreesync can be run as individual component (0xPolygon#188)
  fix: l1infotree flaky test (0xPolygon#182)
  fix: `L1InfoRootIncorrect` error from `agglayer` (0xPolygon#185)
  feat: improve aggsender logs (0xPolygon#186) (0xPolygon#187)
  feat: remove sanity check (0xPolygon#178) (0xPolygon#179)
  refact: GetSequence method (0xPolygon#169)
  feat: epoch notifier (0xPolygon#144)
  feat: unpack and log agglayer errors (0xPolygon#158)
Vui-Chee added a commit to okx/xlayer-cdk that referenced this pull request Dec 4, 2024
* dev: (22 commits)
  fix issues
  feat: healthcheck (#11)
  feat: write on database the number of retries per certificate and the certificates in a history table (0xPolygon#208)
  feat: return an error in case agglayer returns certificate with height lower than in local storage (0xPolygon#209)
  chore: simplify the `HashMeddler` (0xPolygon#205)
  fix: clean proof table on start (0xPolygon#207) (0xPolygon#210)
  feat: improve logs (0xPolygon#204)
  fix: cdk603 error calculating previousLocalExitRoot (0xPolygon#199)
  fix: Integration Bali PP (0xPolygon#198)
  feat: check agglayer certificate and use as initial if db is empty (0xPolygon#192)
  feat: sqlite aggregator (0xPolygon#189)
  feat: BridgeMessage e2e test (0xPolygon#184)
  feat: aggsender e2e (0xPolygon#183)
  fix: aggregating proofs (0xPolygon#191) (0xPolygon#193)
  feat: l1infotreesync can be run as individual component (0xPolygon#188)
  fix: l1infotree flaky test (0xPolygon#182)
  fix: `L1InfoRootIncorrect` error from `agglayer` (0xPolygon#185)
  feat: improve aggsender logs (0xPolygon#186) (0xPolygon#187)
  feat: remove sanity check (0xPolygon#178) (0xPolygon#179)
  refact: GetSequence method (0xPolygon#169)
  ...
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants