ibc: async acknowledgements #7361

fedekunze · 2020-09-22T13:56:32Z

Description

closes: #7254

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.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2020-09-22T14:02:09Z

Codecov Report

Merging #7361 into master will increase coverage by 0.02%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master    #7361      +/-   ##
==========================================
+ Coverage   54.96%   54.99%   +0.02%     
==========================================
  Files         588      588              
  Lines       36631    36664      +33     
==========================================
+ Hits        20135    20163      +28     
- Misses      14406    14409       +3     
- Partials     2090     2092       +2

…7254-async-acks

x/ibc-transfer/keeper/keeper.go

x/ibc/handler.go

x/ibc/04-channel/keeper/packet.go

proto/ibc/lightclients/solomachine/v1/solomachine.proto

x/ibc/09-localhost/types/client_state_test.go

colin-axner

LGTM, left mostly typo fixes and nits

x/ibc/07-tendermint/types/client_state.go

x/ibc/04-channel/types/errors.go

x/ibc/03-connection/keeper/verify.go

x/ibc/03-connection/keeper/verify_test.go

x/ibc/04-channel/keeper/keeper.go

x/ibc/04-channel/keeper/packet.go

x/ibc/light-clients/solomachine/types/proof.go

x/ibc/handler.go

colin-axner · 2020-09-29T12:41:48Z

x/ibc/handler.go

+			// is the case.
+			if ack != nil {
+				if err := k.ChannelKeeper.WriteAcknowledgement(ctx, msg.Packet, ack); err != nil {
+					return nil, err


x/ibc/24-host/keys.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

AdityaSripal

Nice work!! Think there's some issues with how packet receipt is being processed

AdityaSripal · 2020-09-29T16:48:39Z

x/ibc/04-channel/keeper/packet.go

-	if channel.Ordering == types.UNORDERED {
+	switch channel.Ordering {
+	case types.UNORDERED:
+		// check if the packet acknowledgement has been received already for unordered channels
 		_, found := k.GetPacketAcknowledgement(ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())


Shouldn't we be checking for packet receipts now instead of packet acknowledgements?

Yes, this needs to be changed, per the spec - https://github.com/cosmos/ics/tree/master/spec/ics-004-channel-and-packet-semantics#receiving-packets

x/ibc/04-channel/keeper/packet.go

AdityaSripal · 2020-09-29T16:51:55Z

x/ibc/04-channel/keeper/packet_test.go

@@ -280,15 +280,6 @@ func (suite *KeeperTestSuite) TestRecvPacket() {
 			_, _, _, _, channelA, channelB := suite.coordinator.Setup(suite.chainA, suite.chainB, types.UNORDERED)
 			packet = types.NewPacket(validPacketData, 1, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, disabledTimeoutHeight, uint64(suite.chainB.GetContext().BlockTime().UnixNano()))
 		}, false},
-		{"acknowledgement already received", func() {


should be replaced by receipt already written test case

x/ibc/04-channel/keeper/packet.go

x/ibc/04-channel/keeper/packet_test.go

x/ibc/04-channel/keeper/packet.go

x/ibc/07-tendermint/types/client_state_test.go

x/ibc/handler.go

AdityaSripal · 2020-09-29T17:00:31Z

x/ibc/testing/chain.go

+	}
+
+	// commit changes
+	chain.App.Commit()


Is doing this ok? @colin-axner

I think this is ok as it represents the asynchrony of the acknowledgment?

it's ok because no message is being sent to the chain (we are simulating one) and this function is called by the coordinator which immediately increments time afterwards. It isn't great design though (my bad) because it is counter-intuitive. It must be done though because otherwise the state change won't be updated on the client

cwgoes

Thanks; see comments.

x/ibc/04-channel/keeper/keeper.go

cwgoes · 2020-09-29T19:33:57Z

x/ibc/04-channel/keeper/packet.go

-	if channel.Ordering == types.UNORDERED {
+	switch channel.Ordering {
+	case types.UNORDERED:
+		// check if the packet acknowledgement has been received already for unordered channels
 		_, found := k.GetPacketAcknowledgement(ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())


Yes, this needs to be changed, per the spec - https://github.com/cosmos/ics/tree/master/spec/ics-004-channel-and-packet-semantics#receiving-packets

x/ibc/04-channel/keeper/packet.go

Co-authored-by: Aditya <adityasripal@gmail.com>

…sdk into fedekunze/7254-async-acks

…7254-async-acks

AdityaSripal

ACK, Nice work!!!

cwgoes

ACK, nice work 👍

rename packet ack abs

ba1152a

fedekunze added the x/ibc label Sep 22, 2020

fedekunze changed the title ~~ibc: Async acknowledgements~~ ibc: async acknowledgements Sep 22, 2020

fedekunze added 6 commits September 24, 2020 17:48

Merge branch 'master' of github.com:cosmos/cosmos-sdk into fedekunze/…

6f5a4cb

…7254-async-acks

updates

b97f045

update packet executed

c83394d

write ack

f8eef5a

update clients

e9439f9

update transfer keeper

224ebaa

fedekunze commented Sep 28, 2020

View reviewed changes

x/ibc-transfer/keeper/keeper.go Outdated Show resolved Hide resolved

colin-axner reviewed Sep 28, 2020

View reviewed changes

x/ibc/handler.go Outdated Show resolved Hide resolved

colin-axner reviewed Sep 28, 2020

View reviewed changes

x/ibc/handler.go Outdated Show resolved Hide resolved

michaelfig reviewed Sep 28, 2020

View reviewed changes

fedekunze added 4 commits September 29, 2020 11:51

changes from reviews

0e987df

rename ReceiveExecuted -> WriteReceipt

106f04c

tests

4021b0a

merge master

6b67a67

fedekunze marked this pull request as ready for review September 29, 2020 10:35

fedekunze requested review from aaronc, alessio and alexanderbez as code owners September 29, 2020 10:35

fedekunze requested review from cwgoes, colin-axner and AdityaSripal September 29, 2020 10:36

fix tests

c73075a

colin-axner approved these changes Sep 29, 2020

View reviewed changes

Apply suggestions from code review

f35f237

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

AdityaSripal requested changes Sep 29, 2020

View reviewed changes

cwgoes suggested changes Sep 29, 2020

View reviewed changes

fedekunze and others added 7 commits September 29, 2020 22:52

Update x/ibc/07-tendermint/types/client_state_test.go

98865b9

Co-authored-by: Aditya <adityasripal@gmail.com>

comments from review

3d5121d

Merge branch 'fedekunze/7254-async-acks' of github.com:cosmos/cosmos-…

3b0bc48

…sdk into fedekunze/7254-async-acks

update RecvPacket

4f8847c

spec typo

3b32827

Merge branch 'master' of github.com:cosmos/cosmos-sdk into fedekunze/…

1d70d68

…7254-async-acks

test fixes

8d2368c

fedekunze requested review from cwgoes and AdityaSripal September 30, 2020 13:36

AdityaSripal approved these changes Sep 30, 2020

View reviewed changes

fedekunze added 2 commits September 30, 2020 21:29

Merge branch 'master' into fedekunze/7254-async-acks

d577304

Merge branch 'master' into fedekunze/7254-async-acks

1008b28

cwgoes approved these changes Oct 1, 2020

View reviewed changes

fedekunze added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 1, 2020

fedekunze and others added 2 commits October 1, 2020 12:13

conflicts

a6e3aca

Merge branch 'master' into fedekunze/7254-async-acks

f616e61

mergify bot merged commit dcf3b54 into master Oct 1, 2020

mergify bot deleted the fedekunze/7254-async-acks branch October 1, 2020 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ibc: async acknowledgements #7361

ibc: async acknowledgements #7361

fedekunze commented Sep 22, 2020

codecov bot commented Sep 22, 2020 •

edited

colin-axner left a comment

colin-axner Sep 29, 2020

AdityaSripal left a comment

AdityaSripal Sep 29, 2020

cwgoes Sep 29, 2020

AdityaSripal Sep 29, 2020

AdityaSripal Sep 29, 2020

fedekunze Sep 30, 2020

colin-axner Sep 30, 2020

cwgoes left a comment

cwgoes Sep 29, 2020

AdityaSripal left a comment

cwgoes left a comment

ibc: async acknowledgements #7361

ibc: async acknowledgements #7361

Conversation

fedekunze commented Sep 22, 2020

Description

codecov bot commented Sep 22, 2020 • edited

Codecov Report

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 22, 2020 •

edited