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

add: signature command in iovnscli #263

Closed
wants to merge 2 commits into from
Closed

add: signature command in iovnscli #263

wants to merge 2 commits into from

Conversation

fdymylja
Copy link
Contributor

@fdymylja fdymylja commented Jul 9, 2020

No description provided.

@fdymylja fdymylja requested a review from davepuchyr July 9, 2020 21:49
@fdymylja fdymylja self-assigned this Jul 9, 2020
@fdymylja
Copy link
Contributor Author

fdymylja commented Jul 9, 2020

Two notes @davepuchyr , check the signature model if it's fine, I put the text message as bytes so it will be encode in base64 in json, this is to make sure the message stays intact and cycles of marshalling and unmarshalling do not mess with ordering and hence make the sig invalid.

@@ -2,10 +2,6 @@

set -o nounset

echo -n "This script will remove all existing iovns configurations, wallets etc. Do you want to proceed? [Y/n] "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's nice to destroy existing wallets, etc without warning. If this is blocking some CI or script then just pipe yes into it, eg yes | ./init.sh.

@@ -16,5 +16,5 @@ RUN mv /build/iovnscli iovnscli
RUN mv /build/iovnsd iovnsd
COPY ./scripts .
RUN chmod +x init.sh
RUN bash ./init.sh
RUN sh ./init.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be RUN sh -c 'yes | ./init.sh' to address my comment on the deletion from init.sh.

)

type SignatureSchema struct {
ChanID string `json:"@chain_id"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please drop the chain_id as we discussed. It will only add confusion since it's irrelevant for the signing of a message.

ChanID string `json:"@chain_id"`
Type string `json:"@type"`
Message []byte `json:"text"`
Sig string `json:"sig"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the tag to signature.

messageJSON, err := json.Marshal(&SignatureSchema{
ChanID: cliCtx.ChainID,
Type: "message",
Sig: string(sig),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Sig should be base64 encoded so that it looks familiar to users.

Type: "message",
Sig: string(sig),
Message: buf.Bytes(),
PubKey: cliCtx.GetFromAddress(),
Copy link
Contributor

@davepuchyr davepuchyr Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be pubkey, not address, no? We need the curve type, too.

@davepuchyr
Copy link
Contributor

Antoine invigorated Alessio to revisit cosmos/cosmos-sdk#4581, so let's see how that develops before we proceed.

@fdymylja
Copy link
Contributor Author

Closing this for now, eventually we can rivis it in the future

@fdymylja fdymylja closed this Jul 18, 2020
@davepuchyr
Copy link
Contributor

The future arrived quickly. :)

@davepuchyr davepuchyr reopened this Jul 20, 2020
@fdymylja fdymylja removed the blocked label Jul 20, 2020
@davepuchyr
Copy link
Contributor

Obviated by #356.

@davepuchyr davepuchyr closed this Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants