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

chore!: remove merkle library #91

Merged
merged 2 commits into from
Jun 28, 2024
Merged

chore!: remove merkle library #91

merged 2 commits into from
Jun 28, 2024

Conversation

cmwaters
Copy link
Collaborator

This reverts commit fdf1e92.

Closes: #39

@cmwaters cmwaters requested review from a team as code owners June 27, 2024 14:19
@cmwaters cmwaters requested review from staheri14, ninabarbakadze, ramin and distractedm1nd and removed request for a team June 27, 2024 14:19
@celestia-bot celestia-bot requested review from a team June 27, 2024 14:19
@Wondertan
Copy link
Member

Shoot, we haven't decided what we do instead.

@cmwaters
Copy link
Collaborator Author

cmwaters commented Jun 27, 2024

Shoot, we haven't decided what we do instead.

We're actually not using this library atm (we're still depending on the merkle library in celestia-core). I don't mind if we decide to spin out it's own repo, to make merkle it's own go.mod, or just leave it as is

Actually I'm wrong. We do use it so would have to revert back to celestia-core

@rootulp
Copy link
Collaborator

rootulp commented Jun 27, 2024

Note: there are usages of go-square/merkle in celestia-app (e.g. here) so I'm guessing those usages will need to be updated to celestia-core merkle instead when celestia-app bumps to go-square v2.

Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

Seems fine to me. In go-square v2 release notes we should provide guidance to any consumers who use go-square/merkle to migrate to something else (potentially celestia-core merkle)

Copy link
Contributor

@cristaloleg cristaloleg left a comment

Choose a reason for hiding this comment

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

yay

Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Actually I'm wrong. We do use it so would have to revert back to celestia-core

Just saw the edit. I am strictly against go-square depending on celestia-core, even if the merkle pkg is a separate go.mod inside

@rootulp
Copy link
Collaborator

rootulp commented Jun 27, 2024

go-square doesn't need to depend on celestia-core b/c nothing breaks if we remove the merkle package in this repo.

@Wondertan
Copy link
Member

We do use it so would have to revert back to celestia-core

What does revert back mean?

@rootulp
Copy link
Collaborator

rootulp commented Jun 27, 2024

I think consumers of go-square/merkle (i.e. celestia-app) would have to revert back to consuming it from celestia-core.

Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

I see, I misunderstood the message then

@cmwaters cmwaters enabled auto-merge (squash) June 28, 2024 07:54
@celestia-bot celestia-bot requested review from a team June 28, 2024 07:54
@cmwaters cmwaters merged commit 0f26a4e into main Jun 28, 2024
10 checks passed
@cmwaters cmwaters deleted the cal/remove-merkle-lib branch June 28, 2024 07:54
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.

This repo shouldn't maintain Merkle tree implemenation
4 participants