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 cli commands for the coinswap module. #4719

Merged
merged 19 commits into from
Sep 9, 2019

Conversation

aayushijain23
Copy link
Contributor

@aayushijain23 aayushijain23 commented Jul 12, 2019

ref #4443
Adding cli-commands for the coinswap module.
PLEASE NOTE: This PR is up for code review and is not intended to be merged into the master atm.

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: clog add [section] [stanza] [message]

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@aayushijain23 aayushijain23 changed the title Aayushijain23/add cli commands Add cli commands Jul 12, 2019
x/coinswap/internal/keeper/test_common.go Outdated Show resolved Hide resolved
x/coinswap/internal/keeper/test_common.go Outdated Show resolved Hide resolved
x/coinswap/internal/keeper/test_common.go Outdated Show resolved Hide resolved
@aayushijain23 aayushijain23 changed the base branch from master to colin/4443-coinswap July 12, 2019 17:03
@aayushijain23 aayushijain23 changed the title Add cli commands Add cli commands for the coinswap module. Jul 12, 2019
@colin-axner colin-axner mentioned this pull request Jul 23, 2019
5 tasks
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Great work! I left a lot of nit pick stuff and suggestions. Let me know if you have any questions.

x/coinswap/client/cli/query.go Outdated Show resolved Hide resolved
x/coinswap/client/cli/query.go Outdated Show resolved Hide resolved
x/coinswap/client/cli/query.go Outdated Show resolved Hide resolved
x/coinswap/internal/types/querier.go Outdated Show resolved Hide resolved
x/coinswap/client/cli/query.go Show resolved Hide resolved
x/coinswap/client/rest/query.go Outdated Show resolved Hide resolved
x/coinswap/client/rest/query.go Show resolved Hide resolved
x/coinswap/client/rest/query.go Outdated Show resolved Hide resolved
x/coinswap/client/rest/tx.go Show resolved Hide resolved
x/coinswap/client/rest/tx.go Outdated Show resolved Hide resolved
@aayushijain23 aayushijain23 marked this pull request as ready for review July 31, 2019 15:16
x/coinswap/client/cli/query.go Outdated Show resolved Hide resolved
x/coinswap/client/cli/query.go Outdated Show resolved Hide resolved
x/coinswap/client/cli/tx.go Outdated Show resolved Hide resolved
x/coinswap/client/cli/tx.go Outdated Show resolved Hide resolved
x/coinswap/client/cli/tx.go Outdated Show resolved Hide resolved
x/coinswap/client/rest/tx.go Outdated Show resolved Hide resolved
x/coinswap/client/rest/tx.go Outdated Show resolved Hide resolved
x/coinswap/client/rest/tx.go Outdated Show resolved Hide resolved
x/coinswap/client/rest/tx.go Outdated Show resolved Hide resolved
x/coinswap/internal/types/querier.go Show resolved Hide resolved
aayushijain23 and others added 4 commits July 31, 2019 15:33
Co-Authored-By: colin axner <colinaxner@berkeley.edu>
Co-Authored-By: colin axner <colinaxner@berkeley.edu>
Co-Authored-By: colin axner <colinaxner@berkeley.edu>
@colin-axner
Copy link
Contributor

So I think we should add a config file for this client. This would be done the same way gaiad has gaiad.toml, on each query or tx, we read from the config file for the native denom, if it is empty then we do a query for native denom and write it to the config file. This will allow client to have access to native denom without doing an additional query to the connected node. This could be used to require user to specify 100atom on a liquidity deposit despite the face the MsgAddLiquidity only takes in a big.Int for this field and then the cli could return an error if the user doesn't specify the correct denomination in their transaction.

@aayushijain23
Copy link
Contributor Author

So I think we should add a config file for this client. This would be done the same way gaiad has gaiad.toml, on each query or tx, we read from the config file for the native denom, if it is empty then we do a query for native denom and write it to the config file. This will allow client to have access to native denom without doing an additional query to the connected node. This could be used to require user to specify 100atom on a liquidity deposit despite the face the MsgAddLiquidity only takes in a big.Int for this field and then the cli could return an error if the user doesn't specify the correct denomination in their transaction.

I like the idea of storing the native denom in a config file. Also now that the client will have access to the native denom because of the config file, we would be using that to see if the deposit amount (after parsing) has the term atom or not right? And if it doesn't, then we throw an error as you mentioned.
The only problem that I see is that although this solution works for this particular use case, there is a mismatch between the type mentioned and the value that the user gives; i.e. the field only accepts big.Int, but we are expecting the user to add the native denom to it. Is this right way to go about it?

@aayushijain23
Copy link
Contributor Author

As discussed on Slack, will change the type from Big.Int to Coin. This will remove the mismatch between the type expected and the value that the user gives.

aayushijain23 and others added 3 commits August 2, 2019 15:58
Co-Authored-By: colin axner <colinaxner@berkeley.edu>
Co-Authored-By: colin axner <colinaxner@berkeley.edu>
@fedekunze
Copy link
Collaborator

@aayushijain23 mind resolving issues once they are addressed?

@@ -14,6 +14,10 @@ import (
"github.com/cosmos/cosmos-sdk/x/coinswap/internal/types"
)

const (
nativeDenom = "atom"
Copy link
Member

Choose a reason for hiding this comment

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

This should be configurable

Copy link
Contributor

Choose a reason for hiding this comment

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

it is in the core code; its a parameter. The issue on the client is its useful to know what the native denom is but it is inefficient to do an extra query for it for every command especially since it doesn't change after genesis. One way of handling this is through a config file, which could be done at a later point

@fedekunze fedekunze added WIP and removed R4R labels Aug 29, 2019
@fedekunze
Copy link
Collaborator

Let's merge this to the coinswap module PR and address the rest of the comments there

@fedekunze fedekunze merged commit 265f431 into colin/4443-coinswap Sep 9, 2019
@fedekunze fedekunze deleted the aayushijain23/add-cli-commands branch September 9, 2019 13:25
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.

5 participants