General Fee Payment Spec #581
Conversation
|
25800
در تاریخ چهارشنبه ۲ ژوئن ۲۰۲۱، ۲۱:۰۷ emadami ***@***.***>
نوشت:
… ***@***.**** approved this pull request.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#581 (review)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ATM6Q6O7X2K55GKIWEAAXATTQZM3HANCNFSM457CVYMQ>
.
|
There was a problem hiding this comment.
Nice addition.
I am still thinking more about the most secure/failsafe way for the payOnSender info provided by the forward relayer to be validated and returned to the source chain.
More raising questions than anything else. The idea is good, we should just be more specific.
Co-authored-by: Zarko Milosevic <[email protected]>
There was a problem hiding this comment.
Thank you for this very clear proposal. Most of what the specification discusses makes sense; however, I have two primary concerns which I haven't seen discussed (though feel free to route me to discussions if I have missed them):
- This is more of an ergonomic preference, but would it be possible to split this logic out of ICS 4 instead of stuffing it in the core protocol? I think standardising fees definitely makes sense, but I hesitate to introduce more complexity into the core handlers for anything non-essential to correctness or liveness. Could this be a middleware (behaving appropriately on the callbacks) instead?
- It seems to me like including fees like this - without any restriction on the relayer - greatly intensifies the "race condition" - any relayer which sees a potential high fee for relaying a packet will want to try, many relayers will try at once, and a lot of gas / fees will be wasted as a result. IBC already has this problem in a sense, but without fees paid directly for relaying there's much less reason for relayers to compete. I realise there may be legal concerns with specifying a particular relayer who is to receive a fee, but I think we should also at the very least be aware of and conduct a simple analysis of this issue. Maybe @ancazamfir or @milosevic could chime in here as I know Informal has thought about this in the context of Hermes.
Co-authored-by: Christopher Goes <[email protected]>
Co-authored-by: Christopher Goes <[email protected]>
… to chain responsibility
…tart-fee-payment-spec
There was a problem hiding this comment.
This looks quite nice and complete.
I resolved a number of the previous comments that had been addressed and added a number of mostly minor comments here (typos, add extra explanation, etc). Please feel free to resolve those as soon as you push a fix in a new commit.
I will reflect more on the whole middleware design. I like the idea of middleware, but using new ports and versions seems a bit heavy for me. I will take a deeper look and see if I can provide any constructive criticism here and then make another review from that - but outside of the actual middleware implementation, this looks good.
There was a problem hiding this comment.
I do like the middleware idea and do think it is a very elegant alternative to #582 (which I have basically dropped now). Thank you very much for writing this down explicitly. My only remaining questions on the fee middleware was exactly how middleware was supposed to work, so if we all agree on this piece, then the rest of the PR should be quite solid.
From the ics-perspective, we can now easily make 2 apps: ics20-1 and fee-1:ics20-1 which are:
- incompatible with each other
- use the same ics20 logic
- allow compatible chains to connect to either (or both variants)
- allow reuse of
fee-1with other modules (egics27-1).
This is a great acheivement. I have a few points on port selection, but from the chain-chain perspective, this is quite clean. What I do wonder about is user-chain communication. I will break this into multiple points (below):
|
On application-specific "send packet" type messages:
It is aware of both How do we handle composition here? |
|
Following up on that, how would we extend Smart Contracts... In CosmWasm, However, the same issue as above came up. What happens when a contract wishes to create a packet. It must know whether or not it is fee-enabled to make the proper fee payment calls. This no longer becomes transparent to the module, yet it is ignorant of whether or not it is wrapped. This will become quite difficult (and I welcome any suggestions). This is how a CosmWasm contract currently makes an IBC packet. There is still a little time to expand this before we freeze for 1.0, but how? The same applies to CosmWasm contracts using We need good interfaces not just for blockchain clients, but for other modules. |
|
Finally, this whole fee middleware will need to add a user-settable field (Aside) Although there is ideally "backwards compatibility" the way it has been implemented in the Cosmos SDK is so strict, that CosmJS struggles with it. First, they version the type names, and a chain only seems to support one of Also, to ensure the integrity of messages, the SDK will reject any incoming proto message with unknown fields. So even if we use multiple backwards compatible message types as Maybe ibc spec doesn't care about this, but this is a HUGE real world issue that needs to be clarified. |
|
^Note the above comments have been addressed in the |
| if !isCompatible(cpFeeVersion, feeVersion) { | ||
| return error | ||
| } | ||
| selectFeeVersion(cpFeeVersion, feeVersion) |
There was a problem hiding this comment.
What is supposed to happen here?
There was a problem hiding this comment.
This is a middleware-specific function to negotiate the fee versions on each end of the channel. Given the counterparty fee version and the fee version, a final version for both modules is selected.
| channelIdentifier: Identifier, | ||
| counterpartyPortIdentifier: Identifier, | ||
| counterpartyChannelIdentifier: Identifier, | ||
| version: string) { |
There was a problem hiding this comment.
I don't understand exactly the implications on app developer/end user here re this change; do they need to know the version of fee middleware?Can someone clarify this a bit?
There was a problem hiding this comment.
No they will not, since the middleware will strip out its fee-specific version string before passing onto application. Thus, underlying application does not need to know it is being wrapped by middleware
|
|
||
| ```typescript | ||
| function onRecvPacket(packet: Packet, relayer: string): bytes { | ||
| app_acknowledgement = app.onRecvPacket(packet, relayer) |
There was a problem hiding this comment.
I assume relayer here is source address? Why we are passing it to the application?
There was a problem hiding this comment.
The ICS26 packet callbacks interface are now standardized to always send the submitting relayer address which is just message signer. This allows applications to use this information as they wish.
Particularly Fee middleware will take this address and find the corresponding address on source (this must be registered on destination fee middleware prior to packet receive in order to be paid).
| @@ -91,19 +91,19 @@ function onChanCloseConfirm( | |||
| // defined by the module | |||
| } | |||
|
|
|||
| function onRecvPacket(packet: Packet): bytes { | |||
| function onRecvPacket(packet: Packet, relayer: string): bytes { | |||
There was a problem hiding this comment.
Where the relayer comes from? Do we need to add this field to IBC messages?If yes, are there some properties we expect to hold, i.e., is it a problem if a relayer puts a string that is different from it's address?
There was a problem hiding this comment.
This relayer is just the message signer (destination address of relayer). The fee middleware holds a mapping from the destination address to relayer source address which it can put in acknowledgement. If they put wrong source address, they don't get paid. Packet processing is not affected
| // middleware may modify packet | ||
| packet = doCustomLogic(app_packet) | ||
|
|
||
| return ics4.sendPacket(packet) |
There was a problem hiding this comment.
If I am not wrong, we assume that chain of middleware wrappers function as a processing pipeline only upon message reception, but not in other direction (on packet and ack send). Wouldn't it be cleaner if we go through pipeline in both directions?
There was a problem hiding this comment.
This is explaining exactly that concept. On packet/ack receiving; processing goes from ICS-4 to top-layer middleware, through the chain, to base app. On packet/ack sending; processing goes from base application, from lowest to top middleware, and then to ics4.
So the pipeline exists in both directions. The above is an example of middleware that gets called by an underlying base application's SendPacket function. Note it is calling ics4, not the base app
| function onRecvPacket(packet: Packet, relayer: string): bytes { | ||
| doCustomLogic() | ||
|
|
||
| app_acknowledgement = app.onRecvPacket(packet, relayer) |
There was a problem hiding this comment.
Where app comes from?I guess that what we mean here by app is actually anything implementing ICS26 interface (ModuleCallbacks) as we have potentially several wrappers chained together. If I am not wrong, we might need to make some changes to a router module to be able to support chain of wrappers.
There was a problem hiding this comment.
The app here is the underlying application that implements ICS26. It is embedded by the given middleware (how it is embedded is up to specific languages/implementation. In ibc-go, it would be a field in the middleware struct)
The router module should have no changes. The goal is for ICS-4 and the underlying app to be completely oblivious to the existence of the middleware. This is accomplished on the ICS-4 side, since it is still only talking to some ICS26 app which is bound to a unique port. In the middleware case, it is talking to some top-level middleware but it doesn't matter. It's just an ICS-26 app. The middleware itself will route the call further down the chain.
|
One question I have is if wallets like Keplr and hardware wallets (Ledger) can support signing multiMsg transactions. This is necessary for good UX on user side for incentivizing their packets. cc: @charleenfei |
both Keplr and Ledger do support multiMsg tx, I am checking in on Rainbow Wallet. |
Supersedes #580