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(executor): added MsgUpsertSequencer consensus message #1120

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

keruch
Copy link
Contributor

@keruch keruch commented Oct 4, 2024

PR Standards

ADR link https://www.notion.so/dymension/ADR-x-Sequencer-Reward-Address-On-Rollapp-da84af6888c141d0a4c1a8df256a5025
Closes dymensionxyz/dymension#1248


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 changed the base branch from main to feat/consensus-messages October 7, 2024 17:12
@keruch keruch changed the title Kirill/1248 sequencer reward addr feat(executor): added MsgUpsertSequencer consensus message Oct 7, 2024
@keruch keruch marked this pull request as ready for review October 7, 2024 17:13
@keruch keruch requested a review from a team as a code owner October 7, 2024 17:13
Base automatically changed from feat/consensus-messages to main October 9, 2024 13:04
@faultytolly
Copy link
Contributor

I think there should be a way to test it

return []proto.Message{&sequencertypes.MsgUpsertSequencer{
Operator: nextProposerSettlementAddr,
ConsPubKey: anyPubKey,
RewardAddrBytes: addrBytes,
Copy link
Contributor

Choose a reason for hiding this comment

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

For the initial RewardAddress, the settlementAddr is used. Do we need to include both in the consensus message? Is there a scenario where this message can carry two distinct addresses, one Operator and the other Reward address?
Do I understand this correctly, to have a reward address different from the settlement addr, the custom reward address can be set on the rdk manually, in which case the RewardAddrBytes will not override it?
I realize keeping the reward address in the Hub was decided against, but if we recognize the Hub's x/sequencer record to be the source of truth for the sequencer, then we could maybe consider that besides the settlement address, the (custom) reward address could also originate from the Hub's sequencer record. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding your last point. Yes, you are right. That's what we finally decided to do with Omri. We'll enrich the rotation_started event with all the information needed for the sequencer, ie RewardAddr and WhitelistedRelayers so far. And the hub will be the source of truth.

Do we need to include both in the consensus message?

Good point, probably we don't. But as i said above, this will be change anyway soon.

Do I understand this correctly, to have a reward address different from the settlement addr, the custom reward address can be set on the rdk manually, in which case the RewardAddrBytes will not override it?

Currently, RewardAddrBytes overwrites the manually set value. Again, this will change once we accept that the Hub is the source of truce.

@keruch
Copy link
Contributor Author

keruch commented Oct 10, 2024

I think there should be a way to test it

@faultytolly Existing tests already cover the code I added. I even adjusted some existing scenarios to make tests green, you can see the diff.

@keruch
Copy link
Contributor Author

keruch commented Oct 10, 2024

The build fails while the RDK PR dymensionxyz/dymension-rdk#568 is not merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sequencer Reward Address On Rollapp - Impl
4 participants