-
Notifications
You must be signed in to change notification settings - Fork 54
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(sequencers): whitelisted relayers #572
Conversation
a74f418
to
f40c59f
Compare
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.
looks good. left a couple of questions
the task is suspended until the RollApp-Hub ADR is ready. |
the PR is ready for review once again! |
|
||
// Consensus Messages | ||
rpc UpsertSequencer(ConsensusMsgUpsertSequencer) returns (ConsensusMsgUpsertSequencerResponse); |
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.
how this TX prevented from being sent manually on the L2?
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.
as discussed, it can be sent by only by the owner of the address (i.e sequencer can update it's own reward and relayer address which is fine)
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.
I'm not talking about the update msgs, but the UpsertSequencer
which creates new sequencer.
It's supposed to be treated solely as Consesnsus msg
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.
anyway, if it can be run by L2 users, we need to validate the pubkey (as we had in create_sequencer
before).
I prefer to handle it as trusted consensus msg
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.
as discussed gonna block it from running by the L2.
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.
i added a hack what allows the message to be executed only as part of RequestBeginBlocker
.
basically, we say that the only signer is x/seqeucners module addr, thus in practice no one can sign this message. however, while processing consensus msgs, we trigger the execution through MsgServiceRouter
and bypass the ante handler that verifies signatures, thus this msg can be executed as consensus msg.
// GetSigners returns signers of the msg. The only signer is x/sequencers which allows this msg
// to be executed only as part of consensus msgs.
func (m *ConsensusMsgUpsertSequencer) GetSigners() []sdk.AccAddress {
authority := authtypes.NewModuleAddress(ModuleName)
return []sdk.AccAddress{authority}
}
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.
the export genesis is explicitly setting the reward address though we have this already as a field on the sequencer (old code). not sure why?
// ExportGenesis returns the sequencers module's exported genesis.
func (k *Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
genesis := types.DefaultGenesis()
genesis.Params = k.GetParams(ctx)
sequencersAsValidators := k.GetAllSequencers(ctx)
genesis.Sequencers = make([]types.Sequencer, len(sequencersAsValidators))
for i, v := range sequencersAsValidators {
genesis.Sequencers[i].Validator = &v
rewardAddr, ok := k.GetRewardAddr(ctx, v.GetOperator())
if ok {
genesis.Sequencers[i].RewardAddr = rewardAddr.String()
}
}
return genesis
}
Also seems like we're not setting the RelayerAddress collection upon genesis import?
@omritoptix we don't store |
751b67e
to
772c2e2
Compare
Description
ADR link https://www.notion.so/dymension/ADR-Whitelisted-Relayer-Fee-Exemption-in-Rollapp-104a4a51f86a80e8ad94c916c87deb17
Closes dymensionxyz/dymension#1226
Closes #XXX
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
PR review checkboxes:
I have...
Unreleased
section inCHANGELOG.md
godoc
commentsSDK Checklist
map
time.Now()
sendCoin
and notSendCoins
Full security checklist here
For Reviewer:
After reviewer approval: