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

Protobuf definition for Optimint types #73

Merged
merged 2 commits into from
Jun 7, 2021
Merged

Protobuf definition for Optimint types #73

merged 2 commits into from
Jun 7, 2021

Conversation

tzdybal
Copy link
Member

@tzdybal tzdybal commented Jun 3, 2021

As a part of reorganizing of tendermint/spec#53, protobuf definitions as separate PR.
This is re-used code from tendermint/spec#57 (with all the subsequent improvements introduced in tendermint/spec#53).

@tzdybal tzdybal requested review from liamsi and Wondertan June 3, 2021 10:21
@tzdybal tzdybal added this to In progress in Weekly Sprint (core/app/libs) via automation Jun 3, 2021
@tzdybal tzdybal added this to In progress in Cosmos SDK Optimistic Rollups via automation Jun 3, 2021
Copy link
Contributor

@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.

Overall looks good. Left minor comments.

proto/gen.sh Show resolved Hide resolved
proto/optimint.proto Outdated Show resolved Hide resolved
proto/optimint.proto Show resolved Hide resolved
// https://github.com/gogo/protobuf/blob/master/custom_types.md#warnings-and-issues

// Validator
message Validator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to rename this to Aggregator? BlockProducer?
Is there a well-established term in Rollups-land?
cc @adlerjohn @musalbas

Copy link
Member

@adlerjohn adlerjohn Jun 3, 2021

Choose a reason for hiding this comment

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

My personal preference is block producer/proposer. There's no convention at this time unfortunately. But if we're talking about multiple instead of a single one, then it gets weird to use anything except validator.

Copy link
Member

Choose a reason for hiding this comment

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

An alternative, since "validator" has always been a poor word, would be "candidate" and "leader."

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 think we're a bit out-of-context here: Validator is used only in Evidence, and there is always one Validator (in Evidence). And this Evidence is ABCI type re-use here.
As it's only related to Evidence, 'Validator' seems a good name. In Header we use proposer_address, in commit there are just signatures without specifying is it validator/proposer/producer/whatever.

In my opinion there are two options:

  1. Use ABCI Evidence as-is for now, reassess during fraud proofs work.
  2. Create own Evidence type, and I would use "Proposer" instead of Validator.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's keep the ABCI types unmodified. Internally, for types that do not cross the abci boundary, in optimint we can chose a different / better name.

Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Nothing from my side so approve

Cosmos SDK Optimistic Rollups automation moved this from In progress to Reviewer approved Jun 3, 2021
@liamsi liamsi moved this from In progress to Review in progress in Weekly Sprint (core/app/libs) Jun 3, 2021
Copy link
Contributor

@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.

👍🏼

@tzdybal tzdybal merged commit 3bd96a4 into main Jun 7, 2021
Cosmos SDK Optimistic Rollups automation moved this from Reviewer approved to Done Jun 7, 2021
Weekly Sprint (core/app/libs) automation moved this from Review in progress to Done Jun 7, 2021
@tzdybal tzdybal deleted the tzdybal/protobuf branch May 2, 2023 19:40
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.

None yet

4 participants