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

dependency: remove iavl #129

Merged
merged 3 commits into from
Jan 11, 2021
Merged

dependency: remove iavl #129

merged 3 commits into from
Jan 11, 2021

Conversation

tac0turtle
Copy link
Collaborator

@tac0turtle tac0turtle commented Jan 9, 2021

Description

Remove IAVL as a dependency to prevent circular dependencies in ll-app

  • This needs to be tested by the app anyways.

Closes: #128

@tac0turtle tac0turtle added the T:dependencies Type: Pull requests that update a dependency file label Jan 9, 2021
@tac0turtle tac0turtle self-assigned this Jan 9, 2021
@codecov-io
Copy link

codecov-io commented Jan 9, 2021

Codecov Report

Merging #129 (0642c25) into master (0332760) will increase coverage by 0.53%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #129      +/-   ##
==========================================
+ Coverage   60.33%   60.87%   +0.53%     
==========================================
  Files         262      261       -1     
  Lines       23888    23642     -246     
==========================================
- Hits        14414    14393      -21     
+ Misses       7979     7757     -222     
+ Partials     1495     1492       -3     
Impacted Files Coverage Δ
p2p/conn/secret_connection.go 82.98% <ø> (ø)
privval/secret_connection.go 72.68% <ø> (-3.61%) ⬇️
privval/signer_listener_endpoint.go 80.00% <0.00%> (-9.42%) ⬇️
libs/events/events.go 92.94% <0.00%> (-5.89%) ⬇️
consensus/ticker.go 90.00% <0.00%> (-5.00%) ⬇️
privval/socket_listeners.go 78.72% <0.00%> (-4.26%) ⬇️
blockchain/v0/pool.go 77.56% <0.00%> (-3.05%) ⬇️
privval/signer_endpoint.go 78.78% <0.00%> (-3.04%) ⬇️
p2p/pex/pex_reactor.go 78.27% <0.00%> (-0.60%) ⬇️
... and 9 more

@@ -31,9 +32,11 @@ require (
github.com/spf13/cobra v1.1.1
github.com/spf13/viper v1.7.1
github.com/stretchr/testify v1.6.1
github.com/tendermint/tendermint v0.34.0
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼

// require.NoError(t, err)

// assert.NotNil(t, res)
// }
Copy link
Member

@liamsi liamsi Jan 10, 2021

Choose a reason for hiding this comment

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

What is the rationale behind commenting this out vs. deleting the code? Also, I think testOpDecoder and testOp below can be removed or commented out, too.

@evan-forbes can you add an equivalent test into the app repo?

Copy link
Collaborator Author

@tac0turtle tac0turtle Jan 10, 2021

Choose a reason for hiding this comment

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

Wanted to leave it around as an example. I can remove

P.S. The sdk doenst have the notion of a light client, it first needs to be added (cosmos/cosmos-sdk#6563)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think removing makes sense. We can link to the still existing code in the tendermint repo in an issue (over in the app repo or here). Thanks for the context regarding the light client 🙏🏼

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

As expected, there's no "github.com/tendermint/tendermint" in the go mod graph results 👍

While using the replace directive on this branch in the cosmos-sdk fork still produces the same error as before, manually replacing tendermint with lazyledger-core does work! Most, but not all, of the tests fail to build, some of which is due to the PreprocessTxs method to the ABCI interface not yet being ported to the fork of the cosmos-sdk.

thanks, @marbar3778, for fixing this, and saving me from trying an overly complex fix!

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this so quickly Marko!
LGTM

@liamsi liamsi merged commit ace13d9 into celestiaorg:master Jan 11, 2021
@tac0turtle
Copy link
Collaborator Author

Want to enable auto deleting of branches after merges?

@tac0turtle tac0turtle deleted the remove-iavl branch January 11, 2021 09:39
@liamsi
Copy link
Member

liamsi commented Jan 11, 2021

I think it is already on. I think this does not impact branches in PRs from forks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T:dependencies Type: Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove cosmos/iavl from lazyledger-core to stop dependency graph cycle
4 participants