Feat/admin module sdk47 NTRN-70 #2
Conversation
quasisamurai
force-pushed
the
feat/admin-module-sdk47
branch
from
4a6d7c0 to
1b19d62
Compare
quasisamurai
force-pushed
the
feat/admin-module-sdk47
branch
from
1b19d62 to
c46b4db
Compare
quasisamurai
force-pushed
the
feat/admin-module-sdk47
branch
from
04c76e4 to
cc4fc04
Compare
quasisamurai
force-pushed
the
feat/admin-module-sdk47
branch
from
cc4fc04 to
97f3b41
Compare
Co-authored-by: sotnikov-s <[email protected]>
|
|
||
| clientCtx, err := client.GetClientTxContext(cmd) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Literally any x/ (both neutron & non-neutron) cmd function written this way. is there a reason to do this in a single one?
There was a problem hiding this comment.
let's do our best to not increase this chaos. I'd like our code to be well-written. I don't ask you to rewrite all x/ returned errors this way, just to make the ones you write like that
|
is it feasible to fix unit tests for this module? right now i'm getting compilation errors on |
| if m.Proposer == "" { | ||
| return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, m.Proposer) | ||
| } | ||
|
|
There was a problem hiding this comment.
wyt about adding a simple validation here e.g. len(m.Messages) > 0, m.Messages are of sdk.Msg type, maybe something else?
| func (m *MsgSubmitProposalLegacy) SetContent(content govtypesv1b1.Content) error { | ||
| msg, ok := content.(proto.Message) | ||
| if !ok { | ||
| return fmt.Errorf("can't proto marshal %T", msg) |
There was a problem hiding this comment.
can't proto marshal? I think it would be better to phrase it like "failed to cast proposal content of type %T to proto.Message" or something
There was a problem hiding this comment.
This is the way how it's done in current admin module & x/gov in sdk. So there his no point to rewrite legacy code
| // ValidateBasic implements Msg | ||
| func (m *MsgSubmitProposalLegacy) ValidateBasic() error { | ||
| if m.Proposer == "" { | ||
| return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, m.Proposer) |
There was a problem hiding this comment.
what's the point of using an empty address as a description? I'd rather put there something like "empty proposer field"
There was a problem hiding this comment.
This is the way how it's done in current admin module & x/gov in sdk. So there his no point to rewrite legacy code
There was a problem hiding this comment.
I strongly disagree with this statement and I find it distructive. It doesn't matter what is the origin of a bad piece of code, I think we are responsible for making it good as long as we own and maintain it.
NeverHappened
left a comment
NeverHappened
left a comment
There was a problem hiding this comment.
LGTM! Few minor comments.
I also would appreciate create separate tasks for keeper unit tests and renaming SubmitProposal names everywhere
TASK
Admin module PR
Neutron PR
Neutron SDK PR