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

Add IBC fees to IbcMsg::Transfer and ::SendPacket #1749

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

webmaster128
Copy link
Member

No description provided.

@webmaster128 webmaster128 added the Breaking (contracts) Compile-time breaking contracts label Jun 27, 2023
@webmaster128 webmaster128 requested a review from alpe June 27, 2023 08:51
Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Good start! Some comments on expert vs simple use cases.

/// [ICS-29]: https://github.com/cosmos/ibc-go/blob/v7.2.0/docs/middleware/ics29-fee/overview.md
/// [ibc.applications.fee.v1.Fee]: https://github.com/cosmos/ibc-go/blob/v7.2.0/proto/ibc/applications/fee/v1/fee.proto#L11-L31
#[derive(Serialize, Deserialize, Clone, Debug, Default, PartialEq, Eq, JsonSchema)]
pub struct IbcFee {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO This type is good for an advanced devs but I would consider it too complex for default devs. People would need to understand IBC to make proper use of these attributes. As in #1491 (comment) proposed, let's have a single fee field and split the amount in wasmd.

Copy link
Member Author

Choose a reason for hiding this comment

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

People would need to understand IBC to make proper use of these attributes

That's an interesting point. When we are targeting IbcMsg::SendPacket users they better know this. However, IbcMsg::Transfer users do not need to know.

let's have a single fee field and split the amount in wasmd

We can go that route, yeah. However, this assumes wasmd can make better decisions than contract developers here. And right now I don't see how this can be done other than hardcode a split, which we can as well hardcode in cosmwasm-std.

pub ack: Vec<Coin>,
/// The packet timeout fee
pub timeout: Vec<Coin>,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO it makes sense to pass an optional list of relayer addresses here, too so that the contract is in control who can receive incentives for relaying. This may be an advanced use case but should be covered.

Copy link
Member Author

Choose a reason for hiding this comment

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

I left it out in order to keep this type simple. A developer can access to full API by adding a MsgPayPacketFee instead of using this field. Do we really need it here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking (contracts) Compile-time breaking contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants