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.

Sign up for GitHub

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

Jump to bottom

Refactor tx raw commands #2404

Merged
merged 6 commits into from
Jul 14, 2022
Merged

Refactor tx raw commands #2404

merged 6 commits into from
Jul 14, 2022

Conversation

ljoss17
Copy link
Contributor

@ljoss17 ljoss17 commented Jul 13, 2022 •
edited
Loading

Closes: #2315, closes: #2376

Description

This PR refactors the tx raw commands in order to be synchronised with the ADR 010. The following modifications are applied:

  • Remove the raw subcommand from all the tx raw commands
  • Remove the four duplicate commands:
    • tx raw update-client, which is the same as update client
    • tx raw upgrade-client, which is the same as upgrade client
    • tx raw upgrade-clients, which is the same as upgrade clients
    • tx raw create-client, which is the same as create client
  • Replace all flag prefixes --src- and --dst- with more appropriate prefixes
  • Add unit tests for all tx raw commands

PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., guide/).

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@ljoss17 ljoss17 marked this pull request as ready for review July 13, 2022 12:46
seanchen1991
seanchen1991 approved these changes Jul 13, 2022
View reviewed changes
Copy link
Contributor

@seanchen1991 seanchen1991 left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

romac
romac reviewed Jul 13, 2022
View reviewed changes
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Awesome, thanks so much @ljoss17! 💐

Let's rename the guide/src/commands/raw folder to guide/src/commands/tx and we're good to go!

@romac romac self-assigned this Jul 13, 2022
@romac romac merged commit beb4642 into master Jul 14, 2022
@romac romac deleted the luca_joss/refactor_raw_commands branch July 14, 2022 11:43
@romac romac mentioned this pull request Jul 14, 2022
Closed
6 tasks
@ancazamfir
Copy link
Collaborator

ancazamfir commented Jul 14, 2022 •
edited
Loading

I know this is merged but I am not sure about some of these changes.

I find it confusing, especially for handshake Tx-es, where we now use a-chain and b-chain but then in the help strings to make things clear we still use source and destination. For packets seems a bit better but again we use source and destination in the help to explain things.
The general idea for tx (raw) commands is that, regardless of the type of action (relay packet, client or handshake message), you instruct the relayer to send one or more messages to a (destination) chain by retrieving state values and proofs from another (source) chain. So I find using src/ dst more clear here. Or chain/ counterparty.
a-chain and b-chain work fine in the create connection/ channel where you just want to say "create a connection/channel" between chains a and b. And it results in sending Tx-es to both a and b.

I also find it confusing the sender/ receiver in tx upgrade-chain. We use host/ reference in the high level client upgrade. We should use the same here.

Nit: there is stil tx raw packet-recv in the ADR.

@adizere
Copy link
Member

adizere commented Jul 14, 2022 •
edited
Loading

Is it a reasonable compromise if we make the explicit relation between a-chain being a destination always (wherever applicable) and b-chain being the source? I think this might be the case already, we would just document it as such throughout help and guide.

@ancazamfir
Copy link
Collaborator

Is it a reasonable compromise if we make the explicit relation between a-chain being a destination always (wherever applicable) and b-chain being the source? I think this might be the case already, we would just document it as such throughout help and guide.

Not sure what you mean by "make an explicit relation". And btw, a-chain is the source/ counterparty chain (i just looked at all tx CLIs, at least we are consistent).
IMO we should fix this and not use a-chain and b-chain in the tx(raw) and fix the upgrade params names also.

@ancazamfir
Copy link
Collaborator

I propose we use chain/ counterparty instead of b-chain/ a-chain. Except the chain upgrade where we can use host-chain/ reference-chain (with the note that the reference-chain is the chain to be upgraded)

@seanchen1991
Copy link
Contributor

seanchen1991 commented Jul 14, 2022 •
edited
Loading

I propose we use chain/ counterparty instead of b-chain/ a-chain

Just to be clear, are you proposing we remove all a-chain/b-chain language entirely from the CLI?

@ancazamfir
Copy link
Collaborator

Just to be clear, are you proposing we remove all a-chain/b-chain language entirely from the CLI?

Only from the tx CLIs (old tx raw-s)

@ljoss17
Copy link
Contributor Author

ljoss17 commented Jul 15, 2022

As I see the process for the commands syntaxe, it usually has two steps:

  1. One issue/PR to create/update an ADR which generates a discussion up to some mutual agreement
  2. One issue/PR to update the commands in order to match the ADR

So I think this should be discussed in the issue regarding updating ADR 010 #2383 or in a new issue. This would avoid having a mismatch between ADR 010 and the commands, as well as making it easier to track the discussion which lead to the chosen syntaxe.
What do you think ?

@seanchen1991
Copy link
Contributor

seanchen1991 commented Jul 15, 2022 •
edited
Loading

I opened #2410 to capture @ancazamfir's request. We should continue further discussion in there 🙂

hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
@ljoss17
Refactor tx raw commands according to ADR 010 (informalsystems#2404)
d3958e7 
* Remove the `raw` subcommand from all the `tx raw` commands
* Remove the four duplicate commands:
  * `tx raw update-client`, which is the same as `update client`
  * `tx raw upgrade-client`, which is the same as `upgrade client`
  * `tx raw upgrade-clients`, which is the same as `upgrade clients`
  * `tx raw create-client`, which is the same as `create client`
* Replace all flag prefixes `--src-` and `--dst-` with more appropriate prefixes
* Add unit tests for all `tx raw` commands

---

* Updated 'tx raw' commands following ADR 010 update of 07.07.22

* Fixed markdown title in ADR 010

* Added changelog entry

* Updated old 'tx raw' commands with new format in e2e tests and comments

* Renamed folder 'guide/src/commands/raw' to 'guide/src/commands/tx'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romac romac romac left review comments

@seanchen1991 seanchen1991 seanchen1991 approved these changes

@adizere adizere Awaiting requested review from adizere

@ancazamfir ancazamfir Awaiting requested review from ancazamfir

@mzabaluev mzabaluev Awaiting requested review from mzabaluev

@soareschen soareschen Awaiting requested review from soareschen

Assignees

@romac romac

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@ljoss17 @ancazamfir @adizere @seanchen1991 @romac