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

feat(evm): chain parameter acoss all queries/transactions #534

Merged
merged 4 commits into from
Jun 3, 2021

Conversation

jcs47
Copy link
Contributor

@jcs47 jcs47 commented Jun 2, 2021

This PR adds a chain parameter across all queries and transactions of the EVM module

@jcs47 jcs47 requested review from fish-sammy and cgorenflo June 2, 2021 20:55
@jcs47 jcs47 force-pushed the parameterized_evm branch from 893e678 to 64aa313 Compare June 2, 2021 21:01
Copy link
Contributor

@cgorenflo cgorenflo 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 it makes more sense to add the chain to the ethTxCmd and ethQueryCmd (which both need to be renamed) and pass it into its child commands so that we have axelard tx evm [chain] [command] [args] instead of axelard tx evm [command] [chain] [args], otherwise the PR seems fine

@jcs47
Copy link
Contributor Author

jcs47 commented Jun 3, 2021

I think it makes more sense to add the chain to the ethTxCmd and ethQueryCmd (which both need to be renamed) and pass it into its child commands so that we have axelard tx evm [chain] [command] [args] instead of axelard tx evm [command] [chain] [args], otherwise the PR seems fine

I thought about doing that, but wouldn't that only work for chains included in the genesis file? How about chains added on-the-fly? Can we update the cobra commands in run-time? Nevermind, I was thinking about the hard-coded approach we used to have in earlier versions of axelar-core, for the bitcoin networks.

@jcs47
Copy link
Contributor Author

jcs47 commented Jun 3, 2021

I tried to implement Chris suggestion, but it doesn't seem like it is possible: spf13/cobra#610

Since Chris already approved the PR and gave its blessing to merge this PR as is, I am going to go with what we have now

@jcs47 jcs47 merged commit ae11929 into main Jun 3, 2021
@jcs47 jcs47 deleted the parameterized_evm branch June 3, 2021 03:36
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