Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: adding proto files for fee payment middleware #272
feat: adding proto files for fee payment middleware #272
Changes from 5 commits
fb29909
c0c88e9
4d4a8b9
65be512
092f9e9
ddc6410
4565657
ba0e3dc
0bfa303
7cc97fa
85032f9
3e48f92
6671611
ac52989
3e29dab
fd267d0
44c4cdb
488244a
0cedbe5
42cd7d1
0b93645
985a801
1be683d
583e52e
b74ccd5
a0b837e
11fba68
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this package name, it differs from how I did the code layout. It is important to note that proto package names are hard to change, so whatever we choose, it should be intentional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I like this also.
I considered
ibc.applications.middleware.v1.fee.v1
also as potentially the middleware spec could change but 🤷There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proto versioning should be independent of the specs. A v2 middleware spec could use the exact same proto definitions which would still be v1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm starting to think we should drop the
middleware
. Unless we changed our implementation layout and the ICS layout, I don't see why we should differ. It seems possible a module is partially middleware and then we have to have decide whether it is or isn't a middleware module. Also, at some point in the future these proto files should live in cosmos/ibcThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, fair points. I think this makes sense. @AdityaSripal thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is only written on source side, we can use SDK structs here. Should change this to just use sdk.Coins. And I believe we can make this multidenom. cc: @colin-axner @ethanfrey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this. Can you double-check? What do you mean by making this multdenom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just replace these with a
PacketID
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should not be rpcs because they are not called by users. But are functions within the module that are called during IBC packet callbacks. The PayFee rpcs need to be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed, as should its response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed along with response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for consistency we should always specify this is source port and channel. cc: @colin-axner @ethanfrey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would mean a packet is only uniquely identifiable on the source chain, which is the case for 29-fee, but then this seems less compelling to bring into 04-channel since then this is really packet id as identified on the source chain
Why can't the destination chain refer to a packet using the destination port and channel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean both sides get access to source and dest port and channelID. So I think both sides using the same semantics will avoid confusion.
Even a destination chain can refer to packet using its source (counterparty) port/channel. It's mainly about keeping very consistent semantics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see what you're saying, it's possible for collisions on dest chains. So we should make the semantics that the portID/channelID of the PacketID always refers to the port/channel on the chain in question
So source chains refer to packets by source port/channel
dest chains refer to packets by dest port/channel.
Yea let's make this the consistent rule. Good catch @colin-axner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the comment