-
Notifications
You must be signed in to change notification settings - Fork 34
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
Conversation
update proto to lazyleder-core
baseapp: add todo for PreprocessTxs docs
…tendermint function with std lib go mod tidy
…issue causing panics
finish doc sentence
8cabc60
to
3235a74
Compare
Codecov Report
@@ 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
|
Is this ready for another review? |
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? |
Thanks! I'll review more thoroughly this week! Regarding the The second test you mention is the proto_breakage? No need to update to the lastest master yet
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). |
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. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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`. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
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 |
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-coreABCI
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.
this PR only has a template test for the new method added, as the method is not currently implemented.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes