-
Notifications
You must be signed in to change notification settings - Fork 131
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
Fix SS58 Warnings in Message Generator Scripts #865
Conversation
@@ -75,6 +78,7 @@ do | |||
--lane $MESSAGE_LANE \ | |||
--origin Target \ | |||
remark \ | |||
--remark-payload $PAYLOAD \ |
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.
There are a few issues with this (which are mostly introduced in other PRs, but probably must be fixed here):
remark-payload
is marked asconflicts_with("remark_size")
, so you can't (actually can, but see (2)) use both here - in this case the--remark_size
will be ignored;conflicts_with("remark_size")
andconflicts_with("remark_payload")
should actually beconflicts_with("remark-size")
andconflicts_with("remark-payload")
(i.e. underscore -> hyphen). Right now this declaration doesn't work;- since
remark_payload
is now required, I don't see any point in keeping this payload generation. But actually I'd prefer to keep this and markremark_payload
asOptional<>
.
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.
That's incorrect. remark-payload
should not be required, it should be possible to do either:
$0 remark # default remark payload generated
$0 remark --remark-size <SIZE> # remark payload of given size is generated
$0 remark --remark-payload <BYTES> # explicit remark payload is being sent
I'm pretty sure I tested that, but will take a look again and add some test cases to make sure it works correctly, but the change was meant to be 100% backwards compatible.
EDIT: Indeed it does not work correctly, I don't know what the hell I was testing :D #868 to the rescue.
The message generator scripts got missed with some recent CLI updates. This updates themto make sure they indicate a
remark-payload
when neccessary.Tomek sniped these changes (and the fixes suggested by Slava) with #868.
Gonna salvage my green square by morphing this into just fixing some warnings related to wrong SS58 account encodings.