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

use celestiaorg instead of lazyledger #28

Merged
merged 8 commits into from
Sep 1, 2021
Merged

Conversation

evan-forbes
Copy link
Member

@evan-forbes evan-forbes commented Aug 26, 2021

Description

replaces lazyledger-core with celestia-core and updates to the latest commit of celestia-core

blocked by: #522
closes: #26

@evan-forbes evan-forbes self-assigned this Aug 26, 2021
@evan-forbes evan-forbes marked this pull request as draft August 26, 2021 20:41
@@ -116,7 +116,7 @@ func InitTestnet(
) error {

if chainID == "" {
chainID = "chain-" + tmrand.NewRand().Str(6)
chainID = "chain-" + tmrand.Str(6)
Copy link
Member

Choose a reason for hiding this comment

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

I'm just a little bit curios on why do we decide to drop NewRand()? 😶‍🌫️
As latest cosmos-sdk repo is still using NewRand() in their testnet file

Copy link
Member Author

Choose a reason for hiding this comment

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

good question, check out celestiaorg/celestia-core#284

testutil/network/network.go Show resolved Hide resolved
@@ -370,7 +370,7 @@ message Evidence {
];
// Total voting power of the validator set in case the ABCI application does
// not store historical validators.
// https://github.com/lazyledger/lazyledger-core/issues/4581
// https://github.com/celestiaorg/celestia-core/issues/4581
Copy link
Member

Choose a reason for hiding this comment

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

I guess it's better to leave tendermint here as we will be facing 404 error, if navigating with this link

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch!! resolved in 21f0b78

x/evidence/types/query.pb.go Outdated Show resolved Hide resolved
x/evidence/types/query.pb.go Outdated Show resolved Hide resolved
x/ibc/light-clients/07-tendermint/types/tendermint.pb.go Outdated Show resolved Hide resolved
Comment on lines -59 to +58
replace github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.2-alpha.regen.4
replace (
github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.2-alpha.regen.4
github.com/golang/mock => github.com/golang/mock v1.4.4
google.golang.org/grpc => google.golang.org/grpc v1.33.2
)
Copy link
Member Author

Choose a reason for hiding this comment

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

had to use older versions of golang/mock and grpc to get tests to pass.

upstream sdk still uses v1.33.2 of grpc due to the same bugs encountered here ref Should be fixed in a future update.

As for golang/mock, I think an updated version of the sdk will accommodate the updated version, so we can remove that replace when we update.

go.mod Outdated
github.com/btcsuite/btcutil v1.0.2
github.com/btcsuite/btcd v0.22.0-beta
github.com/btcsuite/btcutil v1.0.3-0.20201208143702-a53e38424cce
github.com/celestiaorg/celestia-core v0.0.1-mvp-das-lightclient.0.20210830200007-52e4383a7266
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll also update this to use the master branch as soon as we merge #522

@evan-forbes
Copy link
Member Author

go-lint is expected to fail due to using standard lib instead of tendermint one (see ref.

evan-forbes and others added 2 commits August 30, 2021 15:22
Co-authored-by: Nguyen Nhu Viet <braveryandglory@gmail.com>
@evan-forbes evan-forbes marked this pull request as ready for review August 30, 2021 20:38
@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2021

Codecov Report

Merging #28 (21f0b78) into master (3addd7f) will increase coverage by 0.98%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #28      +/-   ##
==========================================
+ Coverage   61.90%   62.89%   +0.98%     
==========================================
  Files         609      609              
  Lines       35108    40392    +5284     
==========================================
+ Hits        21735    25405    +3670     
- Misses      11088    12664    +1576     
- Partials     2285     2323      +38     
Impacted Files Coverage Δ
baseapp/abci.go 59.86% <ø> (+1.78%) ⬆️
baseapp/baseapp.go 76.27% <ø> (-1.02%) ⬇️
baseapp/grpcrouter.go 87.50% <ø> (+1.45%) ⬆️
baseapp/grpcrouter_helpers.go 31.81% <ø> (+0.23%) ⬆️
baseapp/params.go 100.00% <ø> (ø)
baseapp/test_helpers.go 63.15% <ø> (+6.01%) ⬆️
client/broadcast.go 53.75% <ø> (+1.69%) ⬆️
client/cmd.go 60.33% <ø> (+1.23%) ⬆️
client/context.go 65.18% <ø> (+5.96%) ⬆️
client/flags/flags.go 21.15% <ø> (-0.13%) ⬇️
... and 721 more

@evan-forbes
Copy link
Member Author

evan-forbes commented Aug 31, 2021

also, the liveness test is passing for me locally, so I don't think it's worth debugging the CI atm. If it still fails after we update to the latest version of the sdk, then we can debug it then.

test results
Restarting random docker container simdnode1
Number of Blocks: 6
Number of Blocks: 7
Number of Blocks: 8
Number of Blocks: 8
Number of Blocks: 9
Number of Blocks: 10
Number of Blocks: 11
Number of Blocks: 12
Number of Blocks: 13
Number of Blocks: 14
Restarting random docker container simdnode3
Number of Blocks: 15
Number of Blocks: 16
Number of Blocks: 17
Number of Blocks: 18
Number of Blocks: 19
Number of Blocks: 20
Number of Blocks: 21
Number of Blocks: 22
Number of Blocks: 23
Number of Blocks: 24
Restarting random docker container simdnode3
Number of Blocks: 25
Number of Blocks: 26
Number of Blocks: 26
Number of Blocks: 27
Number of Blocks: 28
Number of Blocks: 29
Number of Blocks: 30
Number of Blocks: 31
Number of Blocks: 32
Number of Blocks: 33
Restarting random docker container simdnode1
Number of Blocks: 34
Number of Blocks: 35
Number of Blocks: 36
Number of Blocks: 37
Number of Blocks: 38
Number of Blocks: 39
Number of Blocks: 40
Number of Blocks: 41
Number of Blocks: 42
Number of Blocks: 43
Restarting random docker container simdnode0
Number of Blocks: 43
Number of Blocks: 44
Number of Blocks: 45
Number of Blocks: 46
Number of Blocks: 46
Number of Blocks: 47
Number of Blocks: 48
Number of Blocks: 49
Number of Blocks: 50
Number of Blocks: 51
Number of blocks reached. Success!

@evan-forbes
Copy link
Member Author

evan-forbes commented Aug 31, 2021

I think we can just update to the latest version of the sdk and see if they fixed the links. leaving a ref #27 to remind myself

Bidon15
Bidon15 previously approved these changes Aug 31, 2021
@Bidon15
Copy link
Member

Bidon15 commented Aug 31, 2021

Let's hope that it will self-cure after the update 🤞 as we will be doomed to do this manual labour omt 😓

Bidon15
Bidon15 previously approved these changes Aug 31, 2021
Copy link
Member

@Bidon15 Bidon15 left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@tzdybal tzdybal left a comment

Choose a reason for hiding this comment

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

LGTM

@evan-forbes evan-forbes merged commit 42cf598 into master Sep 1, 2021
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.

Adjust imports to reflect rebrand
4 participants