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

refactor: separate logic of aggregator with non aggregator #558

Merged

Conversation

mtsitrin
Copy link
Contributor

@mtsitrin mtsitrin commented Jan 3, 2024

from the issue (#552):

current issues in the implementation:

  • multiple redundant events in case of batch submission
    -- HubClient gets events from SL and sends NewSettlementBatchAccepted
    -- stateUpdatesHandler() gets this event and sends additional event EventNewBatchAccepted
  • too complex flow for an aggregator to be synced with correct state height
    -- sequencer sends a batch, but reiles on the internal events to update the syncTarget
    -- aggregator can set the synctarget on it's own after successful SL submit

design changes:

  • aggregator runs produce and submit
  • non-aggregator runs sync and retreive
  • submit loop sets the syncTarget after successful submission (saving 2 internal events)
  • single event type for SL batch acceptance

Code was tested as sequencer, sycning sequencer, and a full node

@mtsitrin mtsitrin linked an issue Jan 3, 2024 that may be closed by this pull request
@mtsitrin mtsitrin requested review from omritoptix and srene January 3, 2024 09:50
@omritoptix
Copy link
Contributor

tests failed on race condition

Copy link

codecov bot commented Feb 4, 2024

Codecov Report

Attention: 110 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (main@996e681). Click here to learn what that means.

Files Patch % Lines
rpc/client/client.go 30.00% 33 Missing and 2 partials ⚠️
mocks/settlement/hubclient.go 5.71% 29 Missing and 4 partials ⚠️
block/submit.go 51.61% 11 Missing and 4 partials ⚠️
settlement/grpc/grpc.go 0.00% 9 Missing ⚠️
block/manager.go 80.00% 6 Missing and 2 partials ⚠️
block/synctarget.go 14.28% 6 Missing ⚠️
settlement/dymension/dymension.go 55.55% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #558   +/-   ##
=======================================
  Coverage        ?   45.99%           
=======================================
  Files           ?      101           
  Lines           ?    15150           
  Branches        ?        0           
=======================================
  Hits            ?     6968           
  Misses          ?     7211           
  Partials        ?      971           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

omritoptix
omritoptix previously approved these changes Feb 4, 2024
Copy link
Contributor

@omritoptix omritoptix left a comment

Choose a reason for hiding this comment

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

beautiful job 💯

@omritoptix
Copy link
Contributor

linters fail

@mtsitrin mtsitrin marked this pull request as ready for review February 4, 2024 19:18
@mtsitrin mtsitrin requested a review from a team as a code owner February 4, 2024 19:18
@mtsitrin mtsitrin changed the base branch from main to fraud_proofs February 4, 2024 19:20
@mtsitrin mtsitrin changed the base branch from fraud_proofs to main February 4, 2024 19:20
@mtsitrin mtsitrin dismissed omritoptix’s stale review February 4, 2024 19:20

The base branch was changed.

@mtsitrin mtsitrin closed this Feb 4, 2024
@mtsitrin mtsitrin reopened this Feb 4, 2024
@mtsitrin mtsitrin requested a review from omritoptix February 4, 2024 19:37
@omritoptix omritoptix merged commit 865d7a3 into main Feb 18, 2024
5 checks passed
@omritoptix omritoptix deleted the mtsitrin/552-separate-logic-of-aggregator-with-non-aggregator branch February 18, 2024 11:29
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.

Separate logic of aggregator with non-aggregator
2 participants