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

feat: remove DS from aggregator #138

Merged
merged 27 commits into from
Oct 28, 2024
Merged

Conversation

ToniRamirezM
Copy link
Contributor

Description

Removes DS from Aggregator.

@ToniRamirezM ToniRamirezM self-assigned this Oct 22, 2024
@ToniRamirezM ToniRamirezM marked this pull request as ready for review October 23, 2024 13:46
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, leaving a couple of comments to consider.

aggregator/aggregator.go Show resolved Hide resolved
aggregator/db/migrations/0004.sql Outdated Show resolved Hide resolved
aggregator/db/migrations/0004.sql Show resolved Hide resolved
aggregator/db/migrations/0004.sql Show resolved Hide resolved
rpc/types/rpcbatch.go Outdated Show resolved Hide resolved
rpc/batch.go Outdated Show resolved Hide resolved
aggregator/aggregator.go Outdated Show resolved Hide resolved
@@ -112,21 +112,18 @@ type Config struct {
// final gas: 1100
GasOffset uint64 `mapstructure:"GasOffset"`

// RPCURL is the URL of the RPC server
RPCURL string `mapstructure:"RPCURL"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it checked that this conf param works with the new templated config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested locally on Kurtosis and it looks ok to me.

UseFullWitness = false
SettlementBackend = "l1"
AggLayerTxTimeout = "5m"
AggLayerURL = "{{AggLayerURL}}"
MaxWitnessRetrievalWorkers = 2
SyncModeOnlyEnabled = false
Copy link
Contributor

Choose a reason for hiding this comment

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

aren't we using this anymore?

Copy link
Contributor Author

@ToniRamirezM ToniRamirezM Oct 28, 2024

Choose a reason for hiding this comment

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

Nop, now the witness is requested on the same thread a prover opens when connecting to the aggregator. CDK-Erigon is protected in case we request to much witnesses with the "busy" logic.

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.

I'm just missing removing the datastremer lib from go.mod, is that possible now?

@ToniRamirezM
Copy link
Contributor Author

ToniRamirezM commented Oct 28, 2024

I'm just missing removing the datastremer lib from go.mod, is that possible now?

Very good point 😅 Just removed it now along an unused file in the state package that was importing it.

Copy link

sonarcloud bot commented Oct 28, 2024

@ToniRamirezM ToniRamirezM merged commit 33a6d4c into develop Oct 28, 2024
11 checks passed
@ToniRamirezM ToniRamirezM deleted the feature/AggregatorNoDS branch October 28, 2024 09:12
ToniRamirezM added a commit that referenced this pull request Nov 6, 2024
* feat: remove DS from aggregator

* feat: unit tests

* fix: rust

* fix: seq-sender tests

* fix: local_config script

* fix: remove unused file

* fix: default config

* fix: test config

* fix: nil l1inforoot

* feat: improve coverage

* fix: comments

* feat: remove DS lib
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants