-
Notifications
You must be signed in to change notification settings - Fork 332
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
Introduce ADR 006: "Hermes release v0.2.0 use-cases" #676
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Provided a few comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few more comments. Also, maybe instead of ibc-0
and ibc-1
we should use <src-chain-id>
and <dst-chain-id>
?
Codecov Report
@@ Coverage Diff @@
## master #676 +/- ##
=========================================
+ Coverage 13.6% 44.0% +30.3%
=========================================
Files 69 150 +81
Lines 3752 9786 +6034
Branches 1374 0 -1374
=========================================
+ Hits 513 4309 +3796
- Misses 2618 5477 +2859
+ Partials 621 0 -621
Continue to review full report at Codecov.
|
length to a minimum. | ||
|
||
To create and update a client: | ||
- `create client <src-chain-id> <dst-chain-id>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, just noticed we have src chain first and then dst chain. Until now we used the other order. First we specify the destination and then the source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I would propose we stop using the src
and dst
for these commands. With the exception of the client, all the others involve multiple transactions so using src/dst doesn't make sense anymore.
Maybe say a
and b
or something else. For connection and channel we can say that open-init
is always sent to a
(if that's the case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still like a bit more:
"create client on X for chain Y" -> create client <chain-x> <chain-y>
instead of
"create client for Y on chain X" -> create client <chain-y> <chain-x>
It's also most consistent with update client
where we only have a chain-x
.
But I can live with either :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- For connection & channel: I like the
a
b
distinction and I think we should make more effort to use this naming scheme consistently. Will use this from now. - For client: cool to use
destination
beforesource
! As a more general comment, I would find it more intuitive and accurate to use the naming schemehost
/target
instead ofdestination
/source
, but I don't insist on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduced the scheme with "a" and "b" 92ffdd1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff!!! Thanks Adi!
…#676) * adr 006 * Finished 1st sketch. Updated changelog. * Cosmetic improvs * Reorder start commands * Introduced establish for finishing handshakes * Added summary * Reviewed commands. Removed start w/ specific client * Review tenses & 'we' plus some nits. * Added client commands * Added create client missing params (optional) * Added port to disambiguate. Separated optional params for better clarity * Mention client create limitation * Reordered connection option * Reviewed * Added entry in TOC * Extended limitations section. Removed delay option from channel/start cmds. * Introduced 'a' and 'b' scheme. Revised * Added the host<>target scheme for client ops.
Closes: #637
Description
Based on discussion from #637, #628, and synchronous coordination with @ancazamfir and @brapse.
Introduces a new ADR 006 documenting these discussions.
ADR 006 rendered
For contributor use:
docs/
) and code comments.Files changed
in the Github PR explorer.