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(settlement): fixed submission bug where multiple events would cancel subscription #842

Merged
merged 16 commits into from
May 16, 2024

Conversation

omritoptix
Copy link
Contributor

@omritoptix omritoptix commented May 10, 2024

This PR solves few issues:

  1. increased buffer for hub events
  2. filtering events
  3. Allow for retries on polling when waiting for batch inclusion

PR Standards

Opening a pull request should be able to meet the following requirements


Close #XXX

<-- Briefly describe the content of this pull request -->

For Author:

  • Targeted PR against correct branch
  • included the correct type prefix in the PR title
  • Linked to Github issue with discussion and accepted design
  • Targets only one github issue
  • Wrote unit and integration tests
  • All CI checks have passed
  • Added relevant godoc comments

For Reviewer:

  • confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • confirmed all author checklist items have been addressed

After reviewer approval:

  • In case targets main branch, PR should be squashed and merged.
  • In case PR targets a release branch, PR should be rebased.

@omritoptix omritoptix requested a review from a team as a code owner May 10, 2024 10:26
Copy link

codecov bot commented May 12, 2024

Codecov Report

Attention: Patch coverage is 35.48387% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 39.86%. Comparing base (851b312) to head (0b652a1).
Report is 33 commits behind head on main.

❗ Current head 0b652a1 differs from pull request most recent head d51eb07. Consider uploading reports for the commit d51eb07 to get more accurate results

Files Patch % Lines
settlement/dymension/dymension.go 37.93% 17 Missing and 1 partial ⚠️
settlement/events.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #842      +/-   ##
==========================================
- Coverage   40.44%   39.86%   -0.59%     
==========================================
  Files         120      119       -1     
  Lines       18935    18738     -197     
==========================================
- Hits         7659     7469     -190     
- Misses      10201    10220      +19     
+ Partials     1075     1049      -26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danwt
Copy link
Contributor

danwt commented May 15, 2024

@omritoptix there is a conflict
is it ready for review afterwards?

@omritoptix
Copy link
Contributor Author

@danwt , @mtsitrin is the owner of this now. I think we should resolve the conflict.

@mtsitrin
Copy link
Contributor

ready for review

settlement/dymension/dymension.go Outdated Show resolved Hide resolved
settlement/dymension/dymension.go Outdated Show resolved Hide resolved
Copy link
Contributor

@danwt danwt left a comment

Choose a reason for hiding this comment

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

I do think it would be good to simplify the big loop

settlement/dymension/dymension.go Outdated Show resolved Hide resolved
settlement/dymension/dymension.go Outdated Show resolved Hide resolved
settlement/dymension/dymension.go Outdated Show resolved Hide resolved
settlement/dymension/dymension.go Outdated Show resolved Hide resolved
settlement/dymension/dymension.go Outdated Show resolved Hide resolved
settlement/dymension/dymension.go Outdated Show resolved Hide resolved
settlement/dymension/dymension.go Show resolved Hide resolved
settlement/dymension/dymension.go Outdated Show resolved Hide resolved
settlement/dymension/dymension.go Outdated Show resolved Hide resolved
@danwt danwt self-requested a review May 16, 2024 13:01
Copy link
Contributor

@danwt danwt left a comment

Choose a reason for hiding this comment

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

nice!

@danwt danwt merged commit f7b9383 into main May 16, 2024
6 checks passed
@danwt danwt deleted the omritoptix/upstream-hotfixes branch May 16, 2024 16:28
omritoptix added a commit that referenced this pull request May 18, 2024
…cel subscription (#842)

Co-authored-by: Michael Tsitrin <[email protected]>
Co-authored-by: Michael Tsitrin <[email protected]>
Co-authored-by: danwt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

multiple events recieved of hub batch acceptance cause subscription to get cancelled
4 participants