Allow zero proof height packet recv ack timeout #2781
Conversation
… non-zero proof height in channel handshake msgs
| name: "get timestamp at height not exists", | ||
| clientState: suite.solomachine.ClientState(), | ||
| height: suite.solomachine.ClientState().GetLatestHeight().Increment(), | ||
| }, |
There was a problem hiding this comment.
this test case was testing the behaviour of GetTimestampAtHeight which would error out when !cs.GetLatestHeight().EQ(height) , which means a zero proof would never be valid.
I'm not sure exactly how to go ahead with this test case or if it's still valid. cc @damiannolan @colin-axner
There was a problem hiding this comment.
We can change the behaviour of GetTimestampAtHeight() to ignore the passed in height, the timestamp returned should simply be the latest timestamp, thus this test case can be removed
| if !cs.GetLatestHeight().EQ(height) { | ||
| return 0, sdkerrors.Wrapf(ErrInvalidSequence, "not latest height (%s)", height) | ||
| } |
| @@ -46,7 +54,7 @@ func TestSoloMachineTestSuite(t *testing.T) { | |||
| suite.Run(t, new(SoloMachineTestSuite)) | |||
| } | |||
|
|
|||
| func (suite *SoloMachineTestSuite) TestSolomachineSetup() { | |||
| func (suite *SoloMachineTestSuite) SetupSolomachine() string { | |||
There was a problem hiding this comment.
I think we should maybe return a SolomachineConfig struct from this function which encapsulates the client/connection/channel/port IDs.
There was a problem hiding this comment.
we could make use of ibctesting.Endpoint
There was a problem hiding this comment.
looking into this now, is looks like the endpoints in all the other tests are constructed using a suite.chainA and suite.chainB, for this will we need to fully create instances of ibctesting.Endpoint that are referencing solomachine and chainA?
There was a problem hiding this comment.
@colin-axner do you think we should do this in this PR or are you happy with a follow up issue? This PR is already pretty big!
There was a problem hiding this comment.
I think any change to use endpoint can be made in a followup pr. I would not recommend modifying the testing package to handle solo machine endpoints, I was primarily thinking that you could manually fill in the endpoint with the correct values without supporting the ability to do suite.coordinator.Setup(path). I see the difficulty though since when you call NewEndpoint you provide quite a bit of information. What do you think about having a NewSolomachineEndpoint, also very open to just creating a new solomachine endpoint struct which contains less information (if that makes sense). I figure we can always consolidate the two types in the future if there is enough overlap
There was a problem hiding this comment.
I think a new type may work well here, I will create a follow up issue for this and look into after this is merged into your branch! 👍
|
I think this is more or less r4r. Pushed some additional changes for Maybe we should add a similar test which updates the solomachine client as well. Can be done in another PR. |
chatton
requested review from
AdityaSripal,
crodriguezvega and
charleenfei
as code owners
| @@ -46,7 +54,7 @@ func TestSoloMachineTestSuite(t *testing.T) { | |||
| suite.Run(t, new(SoloMachineTestSuite)) | |||
| } | |||
|
|
|||
| func (suite *SoloMachineTestSuite) TestSolomachineSetup() { | |||
| func (suite *SoloMachineTestSuite) SetupSolomachine() string { | |||
There was a problem hiding this comment.
we could make use of ibctesting.Endpoint
| // SendPacket mocks sending a packet by setting a packet commitment directly. | ||
| func (solo *Solomachine) SendPacket(chain *TestChain, packet channeltypes.Packet) { | ||
| commitmentHash := channeltypes.CommitPacket(chain.Codec, packet) | ||
| chain.GetSimApp().IBCKeeper.ChannelKeeper.SetPacketCommitment(chain.GetContext(), packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence(), commitmentHash) |
There was a problem hiding this comment.
Ideally we actually send like an ics20 transfer. There was a bug in ibc core channel keeper in relation to solution machine which wouldn't be caught by this, but would be caught by calling ics20 send. I think other clients might look at these tests as inspiration, so might be useful to have a full test. Totally fine with creating a followup issue
There was a problem hiding this comment.
Great suggestion, I'll create a follow up issue for this 👍
|
@chatton You may merge into my branch whenever you feel comfortable (also feel free to set my branch to automerge if I'm away when you do). If you'd like me to merge my branch into main first, just let me know. If you are waiting for an official approval from @damiannolan , that's totally fine, no rush |
chatton
deleted the
damian/cian/2751-allow-zero-proof-height-packet-recv-ack-timeout
branch
Description
closes: #2751
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.