connection and channel identifiers reuse #386
Conversation
No, I am not able to test them. |
There was a problem hiding this comment.
utACK, but I agree with @AdityaSripal, this needs to be tested.
This can be done by adding integrations tests, see test/relayer_gaia_test.go or by manually testing (starting 2 chains and connecting them with a relayer). Writing integration tests is preferred.
For integration tests, I would make a new test for testing reuse. Copy the first part of the gaia relayer test up to client/connection/channel creation. After those are created, save the current values and clear the config and rerun the handshake. Check that the resulting values are the same as the old values. If possible, addressing other test cases mentioned in #383 would be very much appreciated
Feel free to use a different testing approach if you think of something cleaner/more modular
|
@colin-axner , if we clear config then counterparty connection-id will be empty from config. But, when state of connection created previously is OPEN, then we can't reuse that connection as we are checking counterparty connection-id from config with counterparty.PathEnd.ConnectionId in connection details. |
|
@akhilkumarpilli ah yes, it is a little bit more complicated than meets the eye I believe, because of the IBC protocol, if the connection is OPEN, then we can assume the counterparty connection ID has the exact same parameters the counterparty in our path desires (assuming the proposed existing connection ID has a counterparty that matches our counterparty). So if the connection is OPEN, if our counterparty connection ID (on the relayer) is non-empty, it must match, if the counterparty connection ID (on the relayer) is empty and the counterparty set in that connection (on-chain) matches all the parameters of our counterparty (on the relayer) (with exception to connection ID) then we can accept it. cc @AdityaSripal anything I'm missing? |
| case srcConn.Connection.State == conntypes.OPEN && dstConn.Connection.State == conntypes.OPEN: | ||
| last = true | ||
|
|
There was a problem hiding this comment.
@colin-axner, Added this extra condition since both src connection and dst connection states are OPEN and going into infinite loop. Is this right way?
There was a problem hiding this comment.
yes it is! It fixes a bug I was running into previously as well. Great job, great intuition. I'm glad there are tests now.
I redesigned the handshakes with the assumption that we were beginning with empty identifiers and therefore overlooked the case when a user supplies fully set identifiers
There was a problem hiding this comment.
Excellent! This is a great addition. I'd like another review from @AdityaSripal to ensure the new reuse of connection/channels in the OPEN case is safe
Fixes: #349