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

Update ICS-4 to pass relayer/signed to modules #579

Merged
merged 2 commits into from
Jun 3, 2021

Conversation

ethanfrey
Copy link
Contributor

This changes the ibc handler code, but no changes to the packets send over the wire, so non-breaking on the protocol layer.

It provides some needed information to allow fee incentivization for IBC packets, both in the ics20-2 proposal #577 as well as the more general relayer fee proposal #578

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Concept ACK, this seems straightforward.

Just a quick ping @colin-axner - any difficulties here from the ibc-go angle (presumably not)?

@colin-axner
Copy link
Contributor

Just a quick ping @colin-axner - any difficulties here from the ibc-go angle (presumably not)?

Nope, @ethanfrey has already kindly implemented the changes

TimeoutOnClose needs to be updated in this pr though

@ethanfrey
Copy link
Contributor Author

TimeoutOnClose needs to be updated in this pr though

There was also the idea to expose this info for the various channel lifetimes and such, but I would request that in another PR as I see it as a different but related PR.

TimeoutOnClose seems to be more packet-related however and I will add that

Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

concept ACK

Will this PR include wrapping the relayer address in the ack or will this be in a future PR?

@ethanfrey
Copy link
Contributor Author

Will this PR include wrapping the relayer address in the ack or will this be in a future PR?

That is a future and more contraversial point. I am trying to do step by step where I understand there to be consensus already

@ethanfrey
Copy link
Contributor Author

ethanfrey commented Jun 3, 2021

What do I need to get this merged? It has 2/3 approvals and support on the IBC call.

I am happy for more review, but I would like to see a bit faster feedback cycle if there are no objections.
That would let me to build a series of small, easy to review PRs on top of each other.
Otherwise, you just encourage mega-PRs.

In particular, I want to make a PR on acknoweldgement format that should build on this.

@ethanfrey
Copy link
Contributor Author

@milosevic I guess this just needs your review

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.

5 participants