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

Feature/l1bridge2infoindexsync #31

Merged
merged 57 commits into from
Aug 21, 2024

Conversation

arnaubennassar
Copy link
Contributor

@arnaubennassar arnaubennassar commented Aug 5, 2024

Review guide

aggoracle

Reviewers: @rachit77
Notes:

  • Encapsulated the code to setup the env into functions in a new package test/helpers, actual code is unchanged

bridgesync

Reviewers: @goran-ethernal @Stefan-Ethernal
Notes:

  • Addeed config and minor changes to run from cmd
  • Added E2E test using simulator covering the bridge event (claim event still missing)
  • Added missing db tx closing on the processor + debugger has prefix to know if it's the syncer of L1 or L2

claimsponsor

Reviewers: @begmaroman @ToniRamirezM
Notes:

  • new component in charge of sending claims on behalf of the user to the L2

cmd & config

Reviewers: @vcastellm @rachit77
Notes:

  • Added new component RPC
  • Restructured the logic following the pattern of isNeeded?: have all the sub-components declared before the component switch / case so they can be re-used
  • Lots of work to clean the config file, and everything in general. Didn't wanted to go all the way through, but done some cleaning
  • @rachit77 please check that the configs for the "block finality" match with your work on another PR (on the underlaying packages)

l1bridge2infoindexsync

Reviewers: @goran-ethernal @Stefan-Ethernal
Notes:

  • new syncer to track the relation of L1 bridge -> includded on L1 info tree index
  • a bit different to previous syncers that depend strictly on EVM, this one depends on other syncers as well

l1infotreesync

Reviewers: @goran-ethernal @Stefan-Ethernal
Notes:

  • Exposed some methods
  • Update interface for proofs []common.Hash -> [32]common.Hash (this is gonna be a recurring topic, just mentioning here)
  • Added missing rollbacks on processor

lastgersync

Reviewers: @ToniRamirezM @begmaroman
Notes:

  • new syncer to track the injected GERs on L2
  • a bit different to previous syncers as it doesn't relay on events but on eth_calls

reorgdetector

@begmaroman @goran-ethernal FYI I've comented a bunch of code as it was giving problems, and this is being re-worked on another branch

rpc

Reviewers: @vcastellm @rachit77
Notes:

  • RPC server with the endpoints for the bridge_ namespace
  • Have tested it here

sync

Reviewers: @goran-ethernal @Stefan-Ethernal
Notes:

  • Made some stuff public to be reused on other syncers
  • Added log with prefix so we can know from which syncer the logs come from

test/helpers

reviewers: @rachit77
notes: linked to the aggoracle

tree

Reviewers: @goran-ethernal @Stefan-Ethernal
Notes:

  • []common.Hash -> [32]common.Hash
  • Other minor changes and fixes

claimsponsor/claimsponsor.go Show resolved Hide resolved
claimsponsor/claimsponsor.go Outdated Show resolved Hide resolved
claimsponsor/claimsponsor.go Outdated Show resolved Hide resolved
reorgdetector/reorgdetector.go Outdated Show resolved Hide resolved
bridgesync/bridgesync.go Outdated Show resolved Hide resolved
cmd/run.go Show resolved Hide resolved
bridgesync/e2e_test.go Outdated Show resolved Hide resolved
bridgesync/e2e_test.go Show resolved Hide resolved
bridgesync/e2e_test.go Outdated Show resolved Hide resolved
l1infotreesync/processor.go Show resolved Hide resolved
l1infotreesync/processor.go Outdated Show resolved Hide resolved
l1infotreesync/processor.go Outdated Show resolved Hide resolved
l1bridge2infoindexsync/downloader.go Outdated Show resolved Hide resolved
l1bridge2infoindexsync/processor.go Outdated Show resolved Hide resolved
bridgesync/bridgesync.go Outdated Show resolved Hide resolved
bridgesync/bridgesync.go Outdated Show resolved Hide resolved
bridgesync/e2e_test.go Outdated Show resolved Hide resolved
bridgesync/processor.go Outdated Show resolved Hide resolved
l1bridge2infoindexsync/driver.go Outdated Show resolved Hide resolved
l1bridge2infoindexsync/driver.go Outdated Show resolved Hide resolved
l1bridge2infoindexsync/e2e_test.go Show resolved Hide resolved
l1bridge2infoindexsync/l1bridge2infoindexsync.go Outdated Show resolved Hide resolved
l1bridge2infoindexsync/processor.go Outdated Show resolved Hide resolved
l1bridge2infoindexsync/processor.go Outdated Show resolved Hide resolved
@@ -53,6 +59,16 @@ func start(cliCtx *cli.Context) error {
}

components := cliCtx.StringSlice(config.FlagComponents)
l1Client := runL1ClientIfNeeded(components, c.Etherman.URL)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe make more sense to do singleton lazy initialization? then you avoid double knownledge, if you have an error on list of components that require a object could be difficult to find it... it's more simple to create objects when are required:

aggOracle := createAggoracle(*c, l1Client(), l2Client(), l1InfoTreeSync() )

To create the constructor function can use sync.OnceValue. Example:

l1Client:= sync.OnceValue[*ethclient.Client]( func() *ethclient.Client]{
    l1CLient, err := ethclient.Dial(urlRPCL1)
	if err != nil {
		log.Fatal(err)
	}
	return l1CLient
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this sounds like a great idea. Would u mind moving this into another PR tho? This PR is already gigantic, and there is a lot of work to do in terms of config + cmd, I think that your suggestion would be great on that scope?

(if you agree I'd make sure that we have the right ticket for it, and link ur suggestion in there)

rpc/bridge.go Show resolved Hide resolved
Copy link
Contributor

@rachit77 rachit77 left a comment

Choose a reason for hiding this comment

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

LGTM

@Stefan-Ethernal Stefan-Ethernal mentioned this pull request Aug 20, 2024
Copy link
Contributor

@joanestebanr joanestebanr left a comment

Choose a reason for hiding this comment

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

The PR does'nt pass the checks

Copy link

sonarcloud bot commented Aug 21, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@arnaubennassar arnaubennassar merged commit 5294c24 into develop Aug 21, 2024
6 of 9 checks passed
@arnaubennassar arnaubennassar deleted the feature/l1bridge2infoindexsync branch August 21, 2024 10:01
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.

8 participants