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.

Sign up for GitHub

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

Jump to bottom

feat: consensus messages #1093

Merged
merged 15 commits into from
Oct 9, 2024
Merged

feat: consensus messages #1093

merged 15 commits into from
Oct 9, 2024

Conversation

faultytolly
Copy link
Contributor

@faultytolly faultytolly commented Sep 26, 2024 •
edited by keruch
Loading

PR Standards

ADR link https://www.notion.so/dymension/ADR-x-Sequencer-Messages-34f95e2b579e458e8bb4252da19324bd

Closes https://github.com/dymensionxyz/epics/issues/1

PR naming convention: https://hackmd.io/@nZpxHZ0CT7O5ngTp0TP9mg/HJP_jrm7A

<-- Briefly describe the content of this pull request -->

For Author:

  • Targeted PR against correct branch
  • included the correct type prefix in the PR title
  • Linked to Github issue with discussion and accepted design
  • Targets only one github issue
  • Wrote unit and integration tests
  • All CI checks have passed
  • Added relevant godoc comments

For Reviewer:

  • confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • confirmed all author checklist items have been addressed

After reviewer approval:

  • In case targets main branch, PR should be squashed and merged.
  • In case PR targets a release branch, PR should be rebased.

@keruch keruch marked this pull request as ready for review October 2, 2024 17:45
@keruch keruch requested a review from a team as a code owner October 2, 2024 17:45
keruch
keruch requested changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@keruch keruch left a comment

Choose a reason for hiding this comment

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

nice work! left a few comments. my main question is what ConsensusMessagesStream is and which type should implement it.

block/executor.go Show resolved Hide resolved
block/executor.go Outdated Show resolved Hide resolved
block/executor_test.go Show resolved Hide resolved
block/manager.go
mempool,
proxyApp,
eventBus,
nil, // TODO add ConsensusMessagesStream
Copy link
Contributor

Choose a reason for hiding this comment

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

unresolved todo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is only when the feature is implemented. The consensus message stream does not exist now.

proto/types/tendermint/abci/types.proto Show resolved Hide resolved
proto/types/tendermint/abci/types.proto Outdated Show resolved Hide resolved
proto/types/tendermint/types/types.proto Outdated Show resolved Hide resolved
@keruch
Copy link
Contributor

keruch commented Oct 2, 2024

also please resolve merge conflicts

@danwt danwt self-requested a review October 3, 2024 15:40
@keruch
Copy link
Contributor

keruch commented Oct 4, 2024

build fails since we need to merge dymensionxyz/cometbft#9 first

omritoptix
omritoptix reviewed Oct 8, 2024
View reviewed changes
Copy link
Contributor

@omritoptix omritoptix left a comment

Choose a reason for hiding this comment

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

looks good. few minor comments/questions.

block/executor.go Show resolved Hide resolved
types/block.go Show resolved Hide resolved
@omritoptix omritoptix merged commit 51d4178 into main Oct 9, 2024
6 checks passed
@omritoptix omritoptix deleted the feat/consensus-messages branch October 9, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omritoptix omritoptix omritoptix approved these changes

@danwt danwt Awaiting requested review from danwt

@keruch keruch Awaiting requested review from keruch

Assignees

@faultytolly faultytolly

Labels
dym-internal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@faultytolly @keruch @omritoptix