-
Notifications
You must be signed in to change notification settings - Fork 53
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): added MsgUpsertSequencer #568
base: main
Are you sure you want to change the base?
Conversation
build fail |
proto/sequencers/tx.proto
Outdated
@@ -28,10 +29,26 @@ message MsgCreateSequencerResponse { | |||
|
|||
} | |||
|
|||
message MsgUpsertSequencer { |
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.
Could we call this msg ConsensusMsgUpsertSequencer
or something?
x/sequencers/keeper/msg_server.go
Outdated
return nil, err | ||
} | ||
|
||
ctx.EventManager().EmitEvent(sdk.NewEvent( |
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.
Better create an event proto for this and use the EmitTypedEvent
from sdk-utils
. The version of sdk-utils
doesn't have it, it should be added there. I wrote it because the default EmitTypedEvent
from the sdk has a bug with the quotes. We use it in the hub
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.
we can't import sdk-utils
in this repo since the cosmos versions are different. i did a copy of uevent
and added a todo to replace it once the rdk version is updated.
x/sequencers/types/helpers.go
Outdated
"github.com/cosmos/cosmos-sdk/types/bech32" | ||
) | ||
|
||
func BytesToBech32(addr []byte) (string, error) { |
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.
this could maybe be moved to sdk-utils
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.
it can't temporarily, see my comment above
@@ -0,0 +1,71 @@ | |||
// Package uevent is a copy of https://github.com/dymensionxyz/sdk-utils/tree/main/utils/uevent. | |||
// TODO: import sdk-utils directly when the RDK is updated to SDK v0.47 and further. |
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.
we have sdk-util's branch dedicated for the RDK (based on v0.46)
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.
it's deeply outdated :(
// all must-methods are safe to use since they're validated in ValidateBasic | ||
|
||
v := msg.MustValidator() | ||
m.SetSequencer(ctx, v) |
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.
validate it's not already exists?
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.
in the next PR (for Whitelisted Relayer ADR) we'll assume that the hub is the source of truth, so the upsert will become an honest upsert independently of the record existence. i'll keep this code as it is for now if you don't mind, just not to do the extra work of reverting it in the next PR.
@@ -12,6 +11,8 @@ option go_package = "github.com/dymensionxyz/dymension-rdk/x/sequencers/types"; | |||
service Msg { | |||
rpc CreateSequencer(MsgCreateSequencer) returns (MsgCreateSequencerResponse); |
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.
CreateSequencer
should be removed if there's no use case for it
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'll remove it in the next PR as well if you don't mind. i'm gonna refactor the whole msg server there.
x/sequencers/types/helpers.go
Outdated
"github.com/cosmos/cosmos-sdk/types/bech32" | ||
) | ||
|
||
func Bech32ToAddr[T sdk.AccAddress | sdk.ValAddress](addr string) (T, error) { |
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.
move to utils/addressutils
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.
done!
Description
ADR link https://www.notion.so/dymension/ADR-x-Sequencer-Reward-Address-On-Rollapp-da84af6888c141d0a4c1a8df256a5025
Closes dymensionxyz/dymension#1248
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: