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

l2geth: rollup client batch api #516

Merged
merged 5 commits into from
Apr 21, 2021
Merged

l2geth: rollup client batch api #516

merged 5 commits into from
Apr 21, 2021

Conversation

tynes
Copy link
Contributor

@tynes tynes commented Apr 20, 2021

Description
Adds the batch fetching API to the RollupClient so that it can be used to fetch batches from the DTL. Follow up PRs will use this code to sync as the verifier and also to ensure that the L2 transactions match what has been batch submitted as the sequencer.

Additional context
This is required for #477

@changeset-bot
Copy link

changeset-bot bot commented Apr 20, 2021

🦋 Changeset detected

Latest commit: cfa0dae

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@eth-optimism/l2geth Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@tynes tynes added A-geth C-feature Category: features labels Apr 20, 2021
@tynes tynes force-pushed the l2geth/rollup-client-batch branch from 5a965a9 to 1cda714 Compare April 20, 2021 19:22
@tynes tynes requested a review from gakonst April 21, 2021 00:56
Copy link
Contributor

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

LGTM - minor comment

// Batch and its corresponding types.Transactions
func parseTransactionBatchResponse(txBatch *TransactionBatchResponse, signer *types.OVMSigner) (*Batch, []*types.Transaction, error) {
if txBatch == nil {
return nil, nil, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Should return an error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Errors are currently not returned in the case where the element does not exist, but it is possible to do so. The consumer would have to do error comparison for the "element not found" case and then act appropriately. I opted to not do specific error checking client side and just return nil but now that I think about it, it feels like more idiomatic go to have a non-nil return value when the error is nil.

enqueue, err := s.client.GetLastConfirmedEnqueue()
if err != nil {
return fmt.Errorf("Cannot fetch last confirmed queue tx: %w", err)
}
// There are no enqueues yet
if enqueue == nil {
return nil

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've updated the error handling to use errors.Is for error comparison. This is the idiomatic way of doing things, see: https://golang.org/pkg/errors/

Below is from the docs


Is unwraps its first argument sequentially looking for an error that matches the second.
It reports whether it finds a match. It should be used in preference to simple equality checks:

if errors.Is(err, fs.ErrExist)

is preferable to

if err == fs.ErrExist

@tynes tynes force-pushed the l2geth/rollup-client-batch branch 2 times, most recently from 6e45f70 to cfa0dae Compare April 21, 2021 21:48
@tynes tynes merged commit 7e9ca1e into master Apr 21, 2021
@tynes tynes deleted the l2geth/rollup-client-batch branch April 21, 2021 22:12
InoMurko referenced this pull request in omgnetwork/optimism May 25, 2021
* l2geth: add batch querying to rollup client

* l2geth: add patch changeset

* l2geth: complete mock client interface

* l2geth: add comments to rollup client

* l2geth: more idiomatic error handling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature Category: features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants