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

Fix SS58 Warnings in Message Generator Scripts #865

Merged
merged 5 commits into from
Apr 6, 2021

Conversation

HCastano
Copy link
Contributor

@HCastano HCastano commented Apr 5, 2021

The message generator scripts got missed with some recent CLI updates. This updates them
to 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.

@@ -75,6 +78,7 @@ do
--lane $MESSAGE_LANE \
--origin Target \
remark \
--remark-payload $PAYLOAD \
Copy link
Contributor

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):

  1. remark-payload is marked as conflicts_with("remark_size"), so you can't (actually can, but see (2)) use both here - in this case the --remark_size will be ignored;
  2. conflicts_with("remark_size") and conflicts_with("remark_payload") should actually be conflicts_with("remark-size") and conflicts_with("remark-payload") (i.e. underscore -> hyphen). Right now this declaration doesn't work;
  3. 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 mark remark_payload as Optional<>.

Copy link
Contributor

@tomusdrw tomusdrw left a 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.

@HCastano HCastano changed the title Update Message Generator Scripts Fix SS58 Warnings in Message Generator Scripts Apr 6, 2021
@HCastano HCastano requested a review from tomusdrw April 6, 2021 15:08
@tomusdrw tomusdrw merged commit 326e46a into master Apr 6, 2021
@tomusdrw tomusdrw deleted the hc-update-relayer-commands branch April 6, 2021 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants