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

Fix for interval. #1926

Merged
merged 6 commits into from
May 22, 2024
Merged

Fix for interval. #1926

merged 6 commits into from
May 22, 2024

Conversation

StefanIliev545
Copy link
Contributor

Why this change is needed

Please provide a description and a link to the underlying ticket

What changes were made as part of this PR

Please provide a high level list of the changes made

PR checks pre-merging

Please indicate below by ticking the checkbox that you have read and performed the required
PR checks

  • PR checks reviewed and performed

@tudor-malene
Copy link
Collaborator

Why this change is needed

Please provide a description and a link to the underlying ticket

What changes were made as part of this PR

Please provide a high level list of the changes made

PR checks pre-merging

Please indicate below by ticking the checkbox that you have read and performed the required PR checks

  • PR checks reviewed and performed

description pls

Copy link
Collaborator

@tudor-malene tudor-malene 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 not sure if the logic is right

if interval == 0 {
interval = g.blockTime
interval = 20 * time.Second
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this be a default somewhere?
And the log does not match the value

g.logger.Debug("No cross chain data to submit. Skipping.")
continue
}

err = g.sl.L1Publisher().PublishCrossChainBundle(bundle)
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this mean you're publishing even if there is nothing to publish?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it's less than ideal

@@ -286,10 +286,6 @@ func (p *Publisher) PublishCrossChainBundle(bundle *common.ExtCrossChainBundle)
return nil
}

if len(bundle.CrossChainRootHashes) == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

am I missing something, or will this keep publishing every time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the guardian loop continues before calling this when empty

Copy link
Collaborator

Choose a reason for hiding this comment

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

what line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

658
image

Copy link
Collaborator

@tudor-malene tudor-malene left a comment

Choose a reason for hiding this comment

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

lgtm

@StefanIliev545 StefanIliev545 merged commit 762dfef into main May 22, 2024
2 checks passed
@StefanIliev545 StefanIliev545 deleted the siliev/fix-interval branch May 22, 2024 09:20
StefanIliev545 added a commit that referenced this pull request May 22, 2024
* Fix for interval.

* Removed defaulting from guardian as there should already be a default value.

* Made the fromSeqNo to be local.

* Modify how the local cached seq no is set.

* Add missing cfg option.

* Another missing default value.

---------

Co-authored-by: StefanIliev545 <[email protected]>
StefanIliev545 added a commit that referenced this pull request May 22, 2024
* Fix for interval.

* Removed defaulting from guardian as there should already be a default value.

* Made the fromSeqNo to be local.

* Modify how the local cached seq no is set.

* Add missing cfg option.

* Another missing default value.

---------

Co-authored-by: StefanIliev545 <[email protected]>
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.

2 participants