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

Replace tendermint with lazyledger-core #13

Merged
merged 13 commits into from
Jan 21, 2021
Merged

Conversation

evan-forbes
Copy link
Member

@evan-forbes evan-forbes commented Jan 16, 2021

Description

This PR manually replaces the imports from "github.com/tendermint/tendermint" to "github.com/lazyledger/lazyledger-core" in all go and proto files. It also changes the methods of baseapp.BaseApp to adhere to the new lazyledger-core ABCI interface and a few other minor changes to address compile time errors. It should be noted that the new method is not implemented, or well documented and simply passes all transaction data. This PR is required for before #1 can be finished.

closes: #2


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
    this PR only has a template test for the new method added, as the method is not currently implemented.
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

go.mod Show resolved Hide resolved
@evan-forbes evan-forbes force-pushed the evan/replace-tendermint branch from 8cabc60 to 3235a74 Compare January 19, 2021 14:34
@codecov-io
Copy link

Codecov Report

Merging #13 (3235a74) into master (2b01560) will decrease coverage by 0.01%.
The diff coverage is 46.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
- Coverage   61.92%   61.90%   -0.02%     
==========================================
  Files         609      609              
  Lines       35099    35108       +9     
==========================================
+ Hits        21734    21735       +1     
- Misses      11083    11088       +5     
- Partials     2282     2285       +3     
Impacted Files Coverage Δ
baseapp/baseapp.go 77.29% <ø> (ø)
baseapp/grpcrouter.go 86.04% <ø> (ø)
baseapp/grpcrouter_helpers.go 31.57% <ø> (ø)
baseapp/params.go 100.00% <ø> (ø)
baseapp/test_helpers.go 57.14% <ø> (ø)
client/broadcast.go 52.05% <ø> (ø)
client/cmd.go 59.09% <ø> (ø)
client/context.go 59.22% <ø> (ø)
client/flags/flags.go 21.27% <ø> (ø)
client/grpc/tmservice/block.go 50.00% <ø> (ø)
... and 118 more

@liamsi
Copy link
Member

liamsi commented Jan 20, 2021

Is this ready for another review?

@evan-forbes
Copy link
Member Author

evan-forbes commented Jan 20, 2021

I was just about to comment, I think so. The first failing check, Lint/golangci-lint, has to do with file permissions that have not been changed, and are present in the cosmos-sdk, but I can change them if anyone thinks it's a good idea. If not, should this check be kept?

The second is failing because some proto rpc methods were "removed". The check is comparing the master branch of the sdk with ours, and the sdk has added some endpoints since we last updated. I can also update to the latest master branch of the sdk if that is desired. If not, should this check be kept? Altered to check our fork?

@liamsi
Copy link
Member

liamsi commented Jan 20, 2021

Thanks! I'll review more thoroughly this week!

Regarding the Error: G306: Expect WriteFile permissions to be 0600 or less (gosec): which files is it complaining about (I don't see it in the CI output).

The second test you mention is the proto_breakage? No need to update to the lastest master yet

Altered to check our fork?

if that's super easy, why not. Otherwise, I'm OK with ignoring that one for this PR (it just checks if the PR has changes to the protobuf files).

@evan-forbes
Copy link
Member Author

evan-forbes commented Jan 20, 2021

There are quite a few files afaict. Most are in testing, so I don't think those pose that great a threat. There is this one, but I used the same permissions as the cosmos-sdk version.

There are a bunch of instances where the sdk does not follow this rule, even a 0777, which is confusing.

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.

LGTM 💪🏼

// for arbitrary processing steps before transaction data is included in the block.
// todo(evan): update documentation after implemented
func (app *BaseApp) PreprocessTxs(txs abci.RequestPreprocessTxs) abci.ResponsePreprocessTxs {
// TODO(evan): fully implement
Copy link
Member

Choose a reason for hiding this comment

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

You can link to the issue if you want: #5

@@ -132,6 +132,7 @@ sure you are aware of any relevant breaking changes.
* (types) [\#5533](https://github.com/cosmos/cosmos-sdk/pull/5533) Refactored `AppModuleBasic` and `AppModuleGenesis`
to now accept a `codec.JSONMarshaler` for modular serialization of genesis state.
* (types/rest) [\#5779](https://github.com/cosmos/cosmos-sdk/pull/5779) Drop unused Parse{Int64OrReturnBadRequest,QueryParamBool}() functions.
* [\#2](https://github.com/lazyledger/cosmos-sdk/issues/2) Add new abci method `PreprocessTxs` and remove `SetOption`.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for even taking care of tracking changes 👍🏼 Should we keep our changes in a separate file instead?

@@ -26,6 +26,7 @@ require (
github.com/grpc-ecosystem/go-grpc-middleware v1.2.2
github.com/grpc-ecosystem/grpc-gateway v1.16.0
github.com/hashicorp/golang-lru v0.5.4
github.com/lazyledger/lazyledger-core v0.0.0-20210115223437-eff282ad2592
Copy link
Member

Choose a reason for hiding this comment

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

One general remark on the module (the cosmos-sdk one): I was wondering if we should rename it from
module github.com/cosmos/cosmos-sdk to module github.com/lazyledger/cosmos-sdk.
I'm almost surprised to see that the replace directive worked in the ll-app without a problem in this case.

Let's update the app when this is merged and see if this works without any further changes.

@liamsi
Copy link
Member

liamsi commented Jan 21, 2021

Regarding gosec. IMO, it complains in this branch but not in the SDK because, here it recognizes the pattern ioutil.WriteFile while it doesn't with tmos.WriteFile:

 -	err = tmos.WriteFile(file, contents, 0644)
 +	err = ioutil.WriteFile(file, contents, 0644)

This can be silenced by sticking to that tmos import instead. Still weird, especially that 0777 place you mentioned above.

@evan-forbes evan-forbes merged commit 3addd7f into master Jan 21, 2021
@evan-forbes evan-forbes deleted the evan/replace-tendermint branch January 21, 2021 15:27
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.

Switch to lazyledger-core instead of tendermint
3 participants