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

feat!: replace a some same proto message to Tendermint #546

Merged
merged 27 commits into from
Feb 1, 2023

Conversation

zemyblue
Copy link
Member

@zemyblue zemyblue commented Jan 23, 2023

Abstract

This PR remove a same proto messages with Tendemrint in proto file and change to import tendermint lib.

Background

Like #543 , Ostracon should be as compatible as possible with IBC and Cosmos-SDK. And among the proto messages, the same things as Tendermint are re-defined and used as Ostracon proto. Due to these parts, it is very difficult to be compatible with IBC and Cosmos-SDK. Therefore, it is necessary to use Tendermint proto message as identifical as possible.

Detail Changes

Changed

This change is based on Tendermint v0.34.19.
The following proto messages are removed and change to reference Tendermint proto message.

  • abci/types.proto
    • RequestEcho, RequestFlush, RequestInfo, RequestSetOption, RequestInitChain, RequestQuery, RequestCheckTx, RequestDeliverTx, RequestEndBlock, RequestCommit, RequestListSnapshots, RequestOfferSnapshot, RequestLoadSnapshotChunk, RequestApplySnapshotChunk
    • ResponseException, ResponseEcho, ResponseFlush, ResponseInfo, ResponseSetOption, ResponseInitChain, ResponseQuery, ResponseBeginBlock, ResponseDeliverTx, ResponseEndBlock, ResponseCommit, ResponseListSnapshots, ResponseOfferSnapshot, ResponseLoadSnapshotChunk, ResponseApplySnapshotChunk
    • enum CheckTxType, enum Result
    • ConsensusParams, BlockParams, LastCommitInfo, Event, EventAttribute, TxResult
    • Validator, ValidatorUpdate, VoteInfo, EvidenceType, Evidence, Snapshot
  • blockchain/types.proto
    • BlockRequest, NoBlockResponse, StatusRequest, StatusResponse
  • privval/types.proto
    • enum Errors
    • RemoteSignerError, PubKeyRequest, PubKeyResponse, SignVoteRequest, SignedVoteResponse, SignProposalRequest, SignedProposalResponse, PingRequest, PingResponse
  • state/types.proto
    • ValidatorsInfo, ConsensusParamsInfo, Version
  • types/types.proto
    • enum BlockIDFlag, enum SignedMsgType,
    • PartSetHeader, Part, BlockID, Data, Vote, Commit, CommitSig, Proposal, TxProof

Removed

The following proto file is removed. Because all proto messages is same with Tendermint.

  • consensus/types.proto
  • consensus/wal.proto
  • crypto/keys.proto
  • crypto/proof.proto
  • libs/bits/types.proto
  • mempool/types.proto
  • p2p/conn.proto
  • p2p/pex.proto
  • p2p/types.proto
  • statesync/types.proto
  • store/types.proto
  • types/canonical.proto
  • types/events.proto
  • types/params.proto
  • types/validator.proto
  • version/types.proto

Result

The following custom proto file remains. And the related file import path is changed.

  • abci/types.proto
    • Custom RequestBeginBlock and ResponseCheckTx are remained to support custom block header. And RequestBeginRecheckTx and RequestEndRecheckTx request are added.
  • blockchain/types.proto
    • Customized for supporting custom block header.
  • privval/types.proto
    • To query VRFProof
  • rpc/grpc/types.proto
    • ResponseBroadcastTx is changed to support custom block header.
  • state/types.proto
    • redefine State for saving last_proof_hash
  • types/block.proto
    • Redefine Block for custom block header.
  • types/evidence.proto
    • LightClientAttackEvidence is added
  • types/types.proto
    • The block header customize
    • Redefine SignedHeader, LightBlock and BlockMeta for supporting custom block header.

@zemyblue zemyblue self-assigned this Jan 23, 2023
@CLAassistant
Copy link

CLAassistant commented Jan 23, 2023

CLA assistant check
All committers have signed the CLA.

@zemyblue zemyblue marked this pull request as draft January 23, 2023 10:57
@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Merging #546 (2b1fa41) into main (ab43542) will increase coverage by 0.05%.
The diff coverage is 71.16%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #546      +/-   ##
==========================================
+ Coverage   66.10%   66.15%   +0.05%     
==========================================
  Files         278      278              
  Lines       36981    36943      -38     
==========================================
- Hits        24445    24439       -6     
+ Misses      10778    10749      -29     
+ Partials     1758     1755       -3     
Impacted Files Coverage Δ
abci/example/kvstore/kvstore.go 73.83% <0.00%> (ø)
abci/types/application.go 0.00% <0.00%> (ø)
abci/types/messages.go 13.40% <0.00%> (ø)
abci/types/pubkey.go 0.00% <0.00%> (ø)
abci/types/result.go 60.00% <ø> (+41.25%) ⬆️
abci/types/util.go 0.00% <ø> (ø)
blockchain/v2/io.go 0.00% <0.00%> (ø)
blockchain/v2/reactor.go 35.01% <0.00%> (ø)
consensus/msgs.go 84.19% <ø> (ø)
consensus/reactor.go 74.88% <ø> (ø)
... and 125 more

 - ValidatorSet, Validator, SimpleValidator
@Kynea0b Kynea0b added the C: enhancement Classification: New feature or its request, or improvement in maintainability of code label Jan 25, 2023
…nt proto.

  - abci.RequestInitChain
  - abci.ResponseEndBlock
  - abci.LastCommitInfo
  - abci.Validator
  - abci.ValidatorUpdate
  - abci.VoterInfo
  - state.ValidatorsInfo
…rmint proto.

  - privval.PubKeyResponse
  - types.Commit
@zemyblue zemyblue marked this pull request as ready for review January 25, 2023 11:13
@zemyblue zemyblue requested a review from ulbqb January 26, 2023 00:42
Kynea0b
Kynea0b previously approved these changes Jan 30, 2023
Copy link
Contributor

@Kynea0b Kynea0b left a comment

Choose a reason for hiding this comment

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

LGTM

abci/client/client.go Outdated Show resolved Hide resolved
abci/cmd/abci-cli/abci-cli.go Outdated Show resolved Hide resolved
abci/client/local_client.go Outdated Show resolved Hide resolved
abci/example/example_test.go Show resolved Hide resolved
abci/server/server.go Show resolved Hide resolved
blockchain/msgs_test.go Show resolved Hide resolved
privval/msgs.go Outdated Show resolved Hide resolved
privval/signer_client.go Outdated Show resolved Hide resolved
privval/signer_listener_endpoint.go Outdated Show resolved Hide resolved
mempool/clist_mempool_test.go Outdated Show resolved Hide resolved
Apply feed-back

Co-authored-by: Toshimasa Nasu <toshimasa_nasu@hotmail.com>
@zemyblue zemyblue dismissed stale reviews from Kynea0b, ulbqb, and torao via 167616b January 30, 2023 11:15
chore: apply feed-back

Co-authored-by: Toshimasa Nasu <toshimasa_nasu@hotmail.com>
@zemyblue
Copy link
Member Author

OK, If then, @tnasu , is it ok to change all tmprivvalproto to privvalproto and all ostracon's privvalproto to ocprivvalproto?

Other than the ones you pointed out, they are included in the list below.
In this case need to change like above?

  • signer_client_test.go
  • signer_requestHandler.go
  • kms/bench_test.go

Next files has just ostracon's privvalproto.
In this case need to change ocprivvalproto?

  • signer_server.go
  • signer_endpoint.go

@tnasu
Copy link
Member

tnasu commented Jan 31, 2023

@zemyblue

I've checked the below with tmprivvalproto, and I think it's ok to change to privvalproto.

.//test/kms/bench_test.go:      tmprivvalproto "github.com/tendermint/tendermint/proto/tendermint/privval"
.//privval/signer_client.go:    tmprivvalproto "github.com/tendermint/tendermint/proto/tendermint/privval"
.//privval/signer_client_test.go:       tmprivvalproto "github.com/tendermint/tendermint/proto/tendermint/privval"
.//privval/signer_listener_endpoint.go: tmprivvalproto "github.com/tendermint/tendermint/proto/tendermint/privval"
.//privval/msgs.go:     tmprivvalproto "github.com/tendermint/tendermint/proto/tendermint/privval"
.//privval/signer_requestHandler.go:    tmprivvalproto "github.com/tendermint/tendermint/proto/tendermint/privval"

And I've also the below with privvalproto, and I think it's ok to change to ocprivvalproto

.//test/kms/bench_test.go:      privvalproto "github.com/line/ostracon/proto/ostracon/privval"
.//privval/signer_client.go:    privvalproto "github.com/line/ostracon/proto/ostracon/privval"
.//privval/signer_client_test.go:       privvalproto "github.com/line/ostracon/proto/ostracon/privval"
.//privval/signer_listener_endpoint.go: privvalproto "github.com/line/ostracon/proto/ostracon/privval"
.//privval/signer_endpoint.go:  privvalproto "github.com/line/ostracon/proto/ostracon/privval"
.//privval/signer_server.go:    privvalproto "github.com/line/ostracon/proto/ostracon/privval"
.//privval/msgs.go:     privvalproto "github.com/line/ostracon/proto/ostracon/privval"
.//privval/signer_requestHandler.go:    privvalproto "github.com/line/ostracon/proto/ostracon/privval"

privval/signer_client_test.go Outdated Show resolved Hide resolved
privval/signer_requestHandler.go Outdated Show resolved Hide resolved
rpc/client/evidence_test.go Outdated Show resolved Hide resolved
state/execution.go Outdated Show resolved Hide resolved
state/mocks/store.go Show resolved Hide resolved
test/kms/bench_test.go Outdated Show resolved Hide resolved
test/kms/bench_test.go Show resolved Hide resolved
@zemyblue zemyblue merged commit 9d17c23 into Finschia:main Feb 1, 2023
zemyblue added a commit to Finschia/finschia-sdk that referenced this pull request Feb 6, 2023
#869)

* feat!: apply the changes of Finschia/ostracon#546
 - (replace a some same proto message to Tendermint )

* chore: fix lint error

* chore: add changelog

* chore: add more unittest

* chore: increase upgrade_height of cosmovisor test. (5->20)

* fix: test-cosmovisor error

* chore: apply feedback of review.

* docs: update proto format and related document.
@zemyblue zemyblue deleted the change_proto_path3 branch February 7, 2023 10:53
tnasu added a commit to tnasu/ostracon that referenced this pull request Mar 2, 2023
tnasu added a commit to tnasu/ostracon that referenced this pull request Mar 2, 2023
tnasu added a commit to tnasu/ostracon that referenced this pull request Mar 2, 2023
tnasu added a commit to tnasu/ostracon that referenced this pull request Mar 2, 2023
@tnasu tnasu mentioned this pull request Mar 2, 2023
tnasu added a commit that referenced this pull request Mar 7, 2023
* Switch to default/ocabci: #546 (comment)

* Switch to bcproto/ocbcproto: #546 (comment)

* Switch to bcproto/ocbcproto: #546 (comment)

* Switch to types/octypes: #546 (comment)

* Revert to privvalproto

* Format import

* Add test for codecov
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: enhancement Classification: New feature or its request, or improvement in maintainability of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants