Skip to content
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

Remove acknowledgement interface in favour of []byte #5603

Merged
merged 8 commits into from
Feb 18, 2020

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Feb 3, 2020

Closes #5509

@cwgoes cwgoes force-pushed the cwgoes/ics-20-impl-work-contd branch from fe73e87 to 45a4b2c Compare February 18, 2020 15:00
@cwgoes cwgoes changed the title ICS 20 implementation cleanup work contd. Remove acknowledgement interface in favour of []byte Feb 18, 2020
@cwgoes cwgoes marked this pull request as ready for review February 18, 2020 15:54
@@ -64,6 +64,8 @@ func handlePacketDataTransfer(
if err := k.ReceiveTransfer(
ctx, packet.SourcePort, packet.SourceChannel, packet.DestinationPort, packet.DestinationChannel, data,
); err != nil {
// How do we want to handle this case? Maybe we should be more lenient, it's safe to leave the channel open I think.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed here.

Copy link
Member

@jackzampolin jackzampolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but a bit unsure what exactly this changes. This is ack is pending fixed tests.

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one minor comment

x/ibc/20-transfer/types/packet.go Outdated Show resolved Hide resolved
@cwgoes cwgoes merged commit 7dfd6b9 into jack/ics-20 Feb 18, 2020
@cwgoes cwgoes deleted the cwgoes/ics-20-impl-work-contd branch February 18, 2020 21:14
fedekunze added a commit that referenced this pull request Feb 20, 2020
* Add comments, remove unused code and attempt to point to places where the spec is being implemented

* close channel when transfer fails

* rename packer data transfer to align to spec; refactor table tests

* ICS 20 implementation cleanup work (#5602)

* Simulation docs (#5033)

* simulation docs

* update docs with the latest simulation changes

* minor imporvments

* clean up of simulation.md

* expand section on weights

* minor reword

* minor wording fix

Co-authored-by: Marko <marbar3778@yahoo.com>

* Merge PR #5597: Include Amount in Complete Unbonding/Redelegation Events

* Add bank alias for gaia

* Moar bank alias gaia

* Moar bank alias gaia

* Call `TimeoutExecuted`, add wrappers

* Remove unused `MsgRecvPacket`

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-authored-by: Marko <marbar3778@yahoo.com>
Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Jack Zampolin <jack.zampolin@gmail.com>

* Merge PR #5603: Remove acknowledgement interface in favour of []byte

* fixes and cleanup

* spec compliance

* refactor relay prefixes and tests

* Fix test compilation

* cleanup; add notes and additional test case

* Receive transfer test

* Apply suggestions from code review

Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>

* Fix autolinter application

* Add testcase with incorrect prefix

* golangcibot fixes

* delete extra comment

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-authored-by: Christopher Goes <cwgoes@pluranimity.org>
Co-authored-by: Marko <marbar3778@yahoo.com>
Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants