handshake refactor #334
handshake refactor #334
Conversation
| // transaction was executed, log the success or failure using the tx response code | ||
| // NOTE: error is nil, logic should use the returned error to determine if the | ||
| // transaction was successfully executed. | ||
| if res.Code != 0 { |
There was a problem hiding this comment.
I think it makes sense to cut down on the number of Send messages and just handle all the logic in one place
There was a problem hiding this comment.
This is much nicer. We could also add retries for account number in here too...
relayer/connection-tx.go
Outdated
| } | ||
| } | ||
| */ | ||
| // TODO: Query for existing identifier and fill config |
There was a problem hiding this comment.
After talking with @jackzampolin, we want to make the default behaviour of the relayer to reuse existing clients/connection/channels instead of creating a new one by default. This might require adding additional helper functions on the SDK side. For now default will new things, but we can easily add this in later by inserting a function here (or after client creation steps are complete) to fill the potentially empty connection identifiers
There was a problem hiding this comment.
I think this is a nice-to-have. We should get it to work by just creating new identifiers and merge this PR. Future work can implement this feature
|
This pull request introduces 1 alert when merging a18efd0 into 031ecc2 - view on LGTM.com new alerts:
|
|
I'd suggest looking at the file, the diffs are hard to read |
|
This pull request introduces 2 alerts when merging c6492a3 into 031ecc2 - view on LGTM.com new alerts:
|
relayer/channel-tx.go
Outdated
| @@ -12,6 +12,15 @@ import ( | |||
|
|
|||
| // CreateChannel runs the channel creation messages on timeout until they pass | |||
| func (c *Chain) CreateOpenChannels(dst *Chain, maxRetries uint64, to time.Duration) error { | |||
| // client and connection identifiers must be filled in | |||
| if err := ValidateClientPaths(c, dst); err != nil { | |||
There was a problem hiding this comment.
shouldn't this be validate connection paths?
|
|
||
| // GenerateConnHandshakeProof generates all the proofs needed to prove the existence of the | ||
| // connection state on this chain. A counterparty should use these generated proofs. | ||
| func (c *Chain) GenerateConnHandshakeProof(height uint64) (ibcexported.ClientState, []byte, []byte, []byte, clienttypes.Height, error) { |
There was a problem hiding this comment.
Nice, also this looks like easy to retry if the returned proofs are nil.
There was a problem hiding this comment.
named returns would be nice here for docs
| // GenSrcClientID generates the specififed identifier | ||
| func (p *Path) GenSrcClientID() { p.Src.ClientID = RandLowerCaseLetterString(10) } | ||
| func (p *Path) GenSrcClientID() { p.Src.ClientID = "" } |
There was a problem hiding this comment.
Maybe we can pull all that RandLowerCaseLetterString code out...
| @@ -138,7 +138,8 @@ func (r *RelayMsgs) SendWithController(src, dst *Chain, useController bool) { | |||
|
|
|||
| if r.IsMaxTx(msgLen, txSize) { | |||
| // Submit the transactions to src chain and update its status | |||
| r.Succeeded = r.Succeeded && send(src, msgs) | |||
| _, success, _ := src.SendMsgs(msgs) | |||
There was a problem hiding this comment.
That LGTM (the next line preserves the semantics I need).
colin-axner
force-pushed
the
colin/329-handshake-refactor
branch
from
2eed390
to
6719456
Compare
|
This pull request introduces 2 alerts when merging cbba72f into 031ecc2 - view on LGTM.com new alerts:
|
|
This pull request introduces 2 alerts when merging 06b5ae2 into 031ecc2 - view on LGTM.com new alerts:
|
|
This pull request introduces 2 alerts when merging 76b3044 into 031ecc2 - view on LGTM.com new alerts:
|
fix build fix panic
LGTM suppression
* fix upgrade issues * bump commit
bug fixes
fix tests
colin-axner
force-pushed
the
colin/329-handshake-refactor
branch
from
da35191
to
465da7b
Compare
|
I bumped to rc6, cleaned up some of my commits (squashed them down) and manually tested again that this branch still works for base functionality. I'm going to do a regular merge (not using squash) so all the contributors to this branch get credit. I'm merging with integration tests not passing because I don't think that is a sufficient reason to block development/testing/release. We can fix any issues as they arise and fix integration tests in a follow up. I will open issues for things mentioned on this pr that should be addressed in followups |
|
This pull request introduces 2 alerts when merging 465da7b into db25632 - view on LGTM.com new alerts:
|
closes: #329
contains changes from #330, couldn't push that branch to the repo for some reason so I can't open this pr against that one
TODOs: