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: merge base to main #62

Merged
merged 162 commits into from
Feb 10, 2025
Merged

fix: merge base to main #62

merged 162 commits into from
Feb 10, 2025

Conversation

SebastianElvis
Copy link
Member

@SebastianElvis SebastianElvis commented Sep 23, 2024

Resolves #300

This PR merges base branch to main

NOTE: all comments in this PR are addressed in #315

maurolacy and others added 30 commits June 5, 2024 21:17
* Init
* Update babylon-private reference
* Revert "CI: Remove redundant SSH key logic (#218)"
* Add create and check FPs by chain support
* Add consumer FP registration check
* go lint
* Improve error handling
* Make chainID string
* Adjust comment
When error occurs, `block` is `nil` and `block.Height` will cause a nil
pointer panic. Uncovered in local deployment
SebastianElvis and others added 2 commits February 3, 2025 18:00
Resolves #300 

This PR merges the latest `main` to the base branch.

- [x] merge babylonlabs-io/babylon#402
- [x] fix linters / unit tests
- [x] fix e2e

NOTE: CI pipelines using Docker registry do not work for some reason,
asking devops atm. All e2e tests pass locally.
Comment on lines +22 to +32
runs-on: ubuntu-22.04
steps:
- uses: actions/checkout@v3
- name: Set up Go
uses: actions/setup-go@v4
with:
go-version: '1.23'
- name: Run e2e Babylon tests
run: make test-e2e-babylon

e2e_bcd:

Check warning

Code scanning / CodeQL

Workflow does not contain permissions Medium

Actions Job or Workflow does not set permissions
Comment on lines +33 to +43
runs-on: ubuntu-22.04
steps:
- uses: actions/checkout@v3
- name: Set up Go
uses: actions/setup-go@v4
with:
go-version: '1.23'
- name: Run e2e BCD tests
run: make test-e2e-bcd

e2e_op:

Check warning

Code scanning / CodeQL

Workflow does not contain permissions Medium

Actions Job or Workflow does not set permissions
Comment on lines +24 to +34
runs-on: ubuntu-22.04
steps:
- uses: actions/checkout@v3
- name: Set up Go
uses: actions/setup-go@v4
with:
go-version: '1.23'
- name: Run e2e Babylon tests
run: make test-e2e-babylon

e2e_bcd:

Check warning

Code scanning / CodeQL

Workflow does not contain permissions Medium

Actions Job or Workflow does not set permissions
@SebastianElvis SebastianElvis changed the title [DO NOT MERGE] Diff between dev and base branch fix: merge base to main Feb 3, 2025
@SebastianElvis SebastianElvis marked this pull request as ready for review February 3, 2025 11:50
Copy link
Member

@Lazar955 Lazar955 left a comment

Choose a reason for hiding this comment

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

amazing work, will continue reviewing as there's quite a lot of new code for a single session, some comments

Copy link
Member

@gitferry gitferry left a comment

Choose a reason for hiding this comment

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

Great work! I didn't look into details about consumer implementations. Comments mostly about some questions and cleanups. No blocker from my side.

Comment on lines 293 to 294
if res == nil {
return &types.TxResponse{}, nil
Copy link
Member

Choose a reason for hiding this comment

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

how could this case happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was actually introduced at #124 by you 😅 Do you remember any context?

tools/go.mod Outdated
Copy link
Member

Choose a reason for hiding this comment

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

same here

@gitferry
Copy link
Member

gitferry commented Feb 4, 2025

Is this compatible with phase-2?

Copy link
Member

@Lazar955 Lazar955 left a comment

Choose a reason for hiding this comment

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

Still not done with the review, half way through

@SebastianElvis
Copy link
Member Author

SebastianElvis commented Feb 5, 2025

Is this compatible with phase-2?

I think so, as the Babylon interfaces used by FP are not changed between Babylon phase-2/3. The e2e test also passes

Nevertheless, I'm also concerned that this merge might mess up with the FP for phase-2, and would require another round of security audit. In this case @KonradStaniec Should we release FP for phase-2 like what we do in Babylon node?

@gitferry
Copy link
Member

gitferry commented Feb 5, 2025

I think so, as the Babylon interfaces used by FP are not changed between Babylon phase-2/3. The e2e test also passes

Nevertheless, I'm also concerned that this merge might mess up with the FP for phase-2, and would require another round of security audit. In this case @KonradStaniec Should we release FP for phase-2 like what we do in Babylon node?

We probably should do this

@KonradStaniec
Copy link

KonradStaniec commented Feb 5, 2025

Scanned the pr and it seems:

  • that it does not modify the core logic (fp_instance) that much, which is good
  • it adds a lot of of new code, which indeed will be trouble some in case of audits. Not to mention any future incidents etc.

So, I agree that we can't mix integration and v1 effort here.

I think the easiest solution is what @SebastianElvis proposed here i.e

  • create long running release/v1.x branch before merging this pr to main
  • from release/v1.x branch cut the release of v1.0.0-rc.0 finality provider
  • later progress with backports from main as in Babylon repo.

There will be some additional over head, but:

  • the efforts will be neatly separated (i.e v1 FP is strictly for Babylon, v2 will be for Babylon + Integrations)
  • rc.X versions still do not promise stability so we can extend this period for some time.

Does anybody has any other suggestion / ideas ? :)

Copy link
Member

@Lazar955 Lazar955 left a comment

Choose a reason for hiding this comment

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

Amazing work! Let's merge other PR's into this

@SebastianElvis
Copy link
Member Author

  • create long running release/v1.x branch before merging this pr to main
  • from release/v1.x branch cut the release of v1.0.0-rc.0 finality provider
  • later progress with backports from main as in Babylon repo.

Agree with this plan. Will wait for more review and the v1 release before merging this PR.

This PR fixes comments to
#62 and adds
changelog for it
Comment on lines +35 to +45
runs-on: ubuntu-22.04
steps:
- uses: actions/checkout@v3
- name: Set up Go
uses: actions/setup-go@v4
with:
go-version: '1.23'
- name: Run e2e BCD tests
run: make test-e2e-bcd

e2e_op:

Check warning

Code scanning / CodeQL

Workflow does not contain permissions Medium

Actions Job or Workflow does not set permissions
@filippos47 filippos47 merged commit 57fed8d into main Feb 10, 2025
38 checks passed
SebastianElvis added a commit to babylonlabs-io/babylon-integration-deployment that referenced this pull request Feb 19, 2025
Resolves
#31

This PR provides the setup for phase-3 devnet 6.

TODOs

- [x] Tag Babylon snapshot
- [x] Merge babylonlabs-io/finality-provider#62
- [x] Merge babylonlabs-io/btc-staker#114
- [x] Tag btc-staker snapshot
- [x] Tag Babylon contract
- [x] Merge babylonlabs-io/babylon-sdk#80
- [x] Tag Babylon SDK snapshot
- [x] Fix deployment
- [x] Tag Babylon
- [x] Tag Babylon SDK
- [x] Merge babylonlabs-io/finality-provider#335
- [x] Merge babylonlabs-io/finality-provider#333
- [x] Tag finality-provider
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.

merge phase-3 branch into main