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 command generating chain link file #583

Merged
merged 33 commits into from
Aug 14, 2021

Conversation

dadamu
Copy link
Contributor

@dadamu dadamu commented Aug 6, 2021

Description

It is the implementation of #572, it add the command generating chain link file for creating chain link.
Closes #572.

Checklist

  • Targeted PR against correct branch.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit tests.
  • Wrote integration tests (simulation & CLI).
  • Updated the documentation.
  • Added an entry to the CHANGELOG.md file.
  • Re-reviewed Files changed in the Github PR explorer.

@codecov
Copy link

codecov bot commented Aug 6, 2021

Codecov Report

Merging #583 (45149b6) into master (16e1eeb) will increase coverage by 7.02%.
The diff coverage is 72.27%.

❗ Current head 45149b6 differs from pull request most recent head 04afde8. Consider uploading reports for the commit 04afde8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #583      +/-   ##
==========================================
+ Coverage   75.59%   82.62%   +7.02%     
==========================================
  Files          94       92       -2     
  Lines        4519     5524    +1005     
==========================================
+ Hits         3416     4564    +1148     
+ Misses        960      748     -212     
- Partials      143      212      +69     
Impacted Files Coverage Δ
app/desmos/cmd/root.go 0.00% <0.00%> (ø)
x/commons/commons.go 100.00% <ø> (ø)
x/profiles/keeper/msg_server_app_link.go 0.00% <0.00%> (ø)
x/profiles/keeper/msg_server_dtag_transfers.go 74.75% <ø> (-1.72%) ⬇️
x/profiles/keeper/msg_server_relationships.go 100.00% <ø> (ø)
x/profiles/keeper/msgs_server_profile.go 82.22% <ø> (+5.47%) ⬆️
x/profiles/keeper/relay_app_links.go 95.38% <ø> (ø)
x/profiles/keeper/relay_chain_links.go 87.09% <ø> (+0.43%) ⬆️
x/profiles/simulation/decoder.go 63.15% <ø> (-28.85%) ⬇️
x/profiles/types/account.go 56.16% <ø> (+4.75%) ⬆️
... and 132 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b42ce83...04afde8. Read the comment docs.

@dadamu dadamu marked this pull request as ready for review August 9, 2021 04:28
Copy link
Contributor

@leobragaz leobragaz left a comment

Choose a reason for hiding this comment

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

As we discussed it should be useful to have the possibility to create json also for other chains.

// create chain link json
cdc, _ := app.MakeCodecs()
chainLinkJSON := profilescliutils.NewChainLinkJSON(
types.NewBech32Address(addr, app.Bech32MainPrefix),
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like prefix is never used, do you meant to use it here maybe?

chainLinkJSON := profilescliutils.NewChainLinkJSON(
types.NewBech32Address(addr, app.Bech32MainPrefix),
types.NewProof(pubkey, hex.EncodeToString(sig), addr),
types.NewChainConfig(app.Bech32MainPrefix),
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

@RiccardoM
Copy link
Contributor

RiccardoM commented Aug 11, 2021

To be honest I'm not a very big fun of this approach cause it requires the account to be already present inside the keystore in order to be used. What I think it's better is to have a UX that acts like the following:

  1. The user runs a command like desmos create-chain-link

  2. An interactive prompt is displayed, where the user is required to do the following:

    1. Enter the mnemonic of the account they want to link
    2. Select the chain they want to link from a list of the supported ones (we can initially include Cosmos, Osmosis, Akash, etc); or select the Other option.
      If Other is selected, the user is required to input:
      1. The Bech32 prefix
      2. The derivation path of the account
  3. Once everything is selected, we use this data to generate the private key, public key, address and from these to sign the data to create the link.

Here is the complete flow:
Desmos Chain link

For the interactive prompt you can use something like promptui

@dadamu dadamu force-pushed the paul/generate-chainlink-file-cmd branch from 3719208 to 38d0117 Compare August 11, 2021 13:52
@dadamu
Copy link
Contributor Author

dadamu commented Aug 11, 2021

Updated to apply interactive prompt to create chain link file.

@dadamu dadamu requested a review from leobragaz August 12, 2021 05:22
Copy link
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

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

I think that it would be a better UX to not use the --filename flag and, instead, print the result to the command output. This would allow the user to simply print it to any file they want using the > redirection.

app/desmos/cmd/create-chain-link.go Outdated Show resolved Hide resolved
app/desmos/cmd/create-chain-link.go Outdated Show resolved Hide resolved
@dadamu dadamu requested a review from RiccardoM August 12, 2021 07:26
@dadamu
Copy link
Contributor Author

dadamu commented Aug 12, 2021

@RiccardoM The '>' interrupt the interactive prompt. With it, the user can't input the info. I guess we can add filename into prompt as well.

@RiccardoM
Copy link
Contributor

@RiccardoM The '>' interrupt the interactive prompt. With it, the user can't input the info. I guess we can add filename into prompt as well.

Yeah I think it's better to ask the user where to store the file using the prompt in this case, specifying it must be a fully qualified path. Just to avoid having them remember to also use a flag, it's better to keep the UX consistent for all the things

@RiccardoM RiccardoM added this to the v1.0.0 milestone Aug 14, 2021
@RiccardoM RiccardoM merged commit cddb501 into master Aug 14, 2021
@RiccardoM RiccardoM deleted the paul/generate-chainlink-file-cmd branch August 14, 2021 13:40
RiccardoM pushed a commit that referenced this pull request Aug 14, 2021
See PR #583

(cherry picked from commit cddb501)
RiccardoM pushed a commit that referenced this pull request Aug 15, 2021
See PR #583

(cherry picked from commit cddb501)
RiccardoM added a commit that referenced this pull request Sep 2, 2021
* Version 0.17.3

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Update CHANGELOG.md

* Added the on-chain upgrade handler

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Updated Cosmos SDK to fix --dry-run

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Version 0.17.6
- Updated Cosmos to v0.42.8
- Added the upgrade handler for the upcoming on-chain upgrade

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Updated Cosmos SDK to fix the capability issue

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Updated CHANGELOG

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Removed useless replace from within go.mod

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Release workflow update
Fixed tags not being fetched correctly

* Improve pagination

See PR #544

(cherry picked from commit b066424)
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Require chain name to be lowercase

See PR #153

(cherry picked from commit 66e0c98)
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Improved profile validation performance

See PR #557

(cherry picked from commit be23505)
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Added command to generate chain link JSON

See PR #583

(cherry picked from commit cddb501)

* Changed Ledger app name to Desmos

See PR #590

(cherry picked from commit 91b21f7)

* Updated CHANGELOG

* Removed app migration support

* Removed unused Proto file

* Version 1.0.1

* fix: flag indicator in profile save example

* add: flag indicator in example

* Added changeset entry

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
(cherry picked from commit b40144b)

* Version 1.0.2

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

Co-authored-by: Paul <p22626262@gmail.com>
Co-authored-by: Leonardo Bragagnolo <leo.braga95@gmail.com>
Co-authored-by: Wingman L <42913823+ryuash@users.noreply.github.com>
RiccardoM added a commit that referenced this pull request Sep 7, 2021
* Version 0.17.3

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Update CHANGELOG.md

* Added the on-chain upgrade handler

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Updated Cosmos SDK to fix --dry-run

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Version 0.17.6
- Updated Cosmos to v0.42.8
- Added the upgrade handler for the upcoming on-chain upgrade

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Updated Cosmos SDK to fix the capability issue

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Updated CHANGELOG

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Removed useless replace from within go.mod

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Release workflow update
Fixed tags not being fetched correctly

* Improve pagination

See PR #544

(cherry picked from commit b066424)
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Require chain name to be lowercase

See PR #153

(cherry picked from commit 66e0c98)
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Improved profile validation performance

See PR #557

(cherry picked from commit be23505)
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Added command to generate chain link JSON

See PR #583

(cherry picked from commit cddb501)

* Changed Ledger app name to Desmos

See PR #590

(cherry picked from commit 91b21f7)

* Updated CHANGELOG

* Removed app migration support

* Removed unused Proto file

* Version 1.0.1

* fix: flag indicator in profile save example

* add: flag indicator in example

* Added changeset entry

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
(cherry picked from commit b40144b)

* Version 1.0.2

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* fix: added missing \ in profile save command example (#602)

(cherry picked from commit 3a73c95)

* Version v1.0.3

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

Co-authored-by: Paul <p22626262@gmail.com>
Co-authored-by: Leonardo Bragagnolo <leo.braga95@gmail.com>
Co-authored-by: Wingman L <42913823+ryuash@users.noreply.github.com>
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.

Add command to generate chain link file
3 participants