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.

Sign up for GitHub

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

Jump to bottom

feat: removes syncers for aggkit migration #256

Open
wants to merge 16 commits into
base: feat/aggkit-repo-migration
Choose a base branch
from
Open

feat: removes syncers for aggkit migration #256

wants to merge 16 commits into from
+181 −21,689

Conversation

rachit77
Copy link
Contributor

@rachit77 rachit77 commented Jan 7, 2025

Description

This PR removes syncers and other relevant packages for aggkit repo migration

@rachit77 rachit77 changed the title Removes syncers for aggkit migration feat: removes syncers for aggkit migration Jan 7, 2025
@rachit77 rachit77 marked this pull request as draft January 7, 2025 13:22
@rachit77 rachit77 self-assigned this Jan 10, 2025
@rachit77 rachit77 marked this pull request as ready for review January 10, 2025 14:52
@Stefan-Ethernal Stefan-Ethernal force-pushed the feat/aggkit-repo-migration branch from 79c94e2 to 04b8363 Compare January 21, 2025 11:37
@Stefan-Ethernal Stefan-Ethernal mentioned this pull request Jan 21, 2025
Draft
3 tasks
@Stefan-Ethernal Stefan-Ethernal force-pushed the rachit77/remove-syncers branch from d814581 to ac8d966 Compare January 27, 2025 10:51
@rachit77 rachit77 requested a review from a team January 28, 2025 14:15
goran-ethernal
goran-ethernal reviewed Feb 3, 2025
View reviewed changes
config/default.go
@@ -276,16 +276,6 @@ RetryAfterErrorPeriod = "1s"
MaxRetryAttemptsAfterError = -1
WaitForNewBlocksPeriod = "3s"

[LastGERSync]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to discuss about the config we will leave in the cdk repo on the office hours. Maybe it makes sense to remove bunch of stuff.

Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If we run the bridge service from the CDK, we would need to revert this change and include the LastGERSync configuration.

rpc/bridge.go Outdated
"github.com/0xPolygon/cdk/rpc/types"
tree "github.com/0xPolygon/cdk/tree/types"
"github.com/agglayer/aggkit/claimsponsor"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be one more topic to discuss. Should bridge rpc endpoints be left in the cdk or in the aggkit repo.

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 think bridge rpc endpoints should be on aggkit but yeah we can discuss this

@Stefan-Ethernal Stefan-Ethernal force-pushed the feat/aggkit-repo-migration branch from baa4aaa to 7116e38 Compare February 4, 2025 14:50
@Stefan-Ethernal
Copy link
Collaborator

Should we reuse the following packages from the aggkit as well (and remove it from the cdk):

  • db,
  • etherman (partially),
  • hex,
  • log

Stefan-Ethernal and others added 8 commits February 10, 2025 01:36
@Stefan-Ethernal @rachit77
feat: remove aggoracle component (#251)
3d4be0e 
* remove aggoracle component

* improve Rust build perf and cleanup

* handle error when unmarashaling versions.json
@Stefan-Ethernal @rachit77
feat: remove aggsender component (#252)
5d32d87 
* remove aggsender component

* fix: remove build-tools
@temaniarpit27 @Stefan-Ethernal @rachit77
feat: mockery generate (#255)
5088af4 
* mockery generate

* Update .mockery.yaml

Co-authored-by: Stefan Negovanović <[email protected]>

* review comments

* review comment

---------

Co-authored-by: Stefan Negovanović <[email protected]>
@rachit77
wip
327a72f
@ToniRamirezM @rachit77
feat: fix BuildVersions during cargo build (#258)
6f7fd29
@rachit77
fix DAC imports
800570f
@rachit77
remove unused code
5c8d613
@Stefan-Ethernal @rachit77
chore: remove empty docker-compose.yml
1cd27ec
@rachit77 rachit77 force-pushed the rachit77/remove-syncers branch from 327487e to 1cd27ec Compare February 11, 2025 09:55
@rachit77
Copy link
Contributor Author

rachit77 commented Feb 11, 2025 •
edited
Loading

Should we reuse the following packages from the aggkit as well (and remove it from the cdk):

  • db,
  • etherman (partially),
  • hex,
  • log

Removed db, hex and log. For etherman maybe we need discuss how we want to tackle this

@Stefan-Ethernal
Copy link
Collaborator

Stefan-Ethernal commented Feb 11, 2025 •
edited
Loading

Just to put a reminder here:
we should remove the bridge rpc implementation logic from the CDK (https://github.com/0xPolygon/cdk/blob/327a72f495a7f4ef27138cf58ec629bc8c2e30ac/rpc/bridge.go), but we might want to instantiate the one from the aggkit repository (TBD). The same applies to the aggsender component.

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stefan-Ethernal Stefan-Ethernal Stefan-Ethernal left review comments

@goran-ethernal goran-ethernal goran-ethernal left review comments

Assignees

@rachit77 rachit77

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rachit77 @Stefan-Ethernal @goran-ethernal @temaniarpit27 @ToniRamirezM