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 destination info in FeeManager::handle_fee #1549

Closed
franciscoaguirre opened this issue Sep 13, 2023 · 6 comments
Closed

Add destination info in FeeManager::handle_fee #1549

franciscoaguirre opened this issue Sep 13, 2023 · 6 comments
Assignees

Comments

@franciscoaguirre
Copy link
Contributor

In #1234, XcmFeesToAccount was added, which puts the delivery fees in a specified account.

A comment was raised about the benefit of putting the fees on different accounts based on the destination of the transport protocol.
However, this needs a change in the FeeManager API.
Right now FeeManager::handle_fee doesn't take in the destination, and it should to make this possible.

Something like this:

fn handle_fee(fee: MultiAssets, context: Option<&XcmContext>, destination: Option<&MultiLocation>);
@franciscoaguirre franciscoaguirre changed the title Allow a different account to gather fees for bridge hubs Add destination info in FeeManager::handle_fee Sep 13, 2023
@KiChjang
Copy link
Contributor

The original comment is really looking for the ID of the bridge that is going to be used to send XCMs over. Given that's the case, instead of modifying the function signature, we could simply pass in the bridge ID as part of the XcmContext -- I imagine that the ID would be useful in other situations as well.

@serban300
Copy link
Contributor

Not sure if possible (didn't try it), but another option that comes to mind would be to modify handle_fee() in order to provide the fee reason. Something like this:

fn handle_fee(fee: MultiAssets, context: Option<&XcmContext>, reason: FeeReason);

And add the destination to the FeeReason variants where it makes sense (for example to FeeReason::TransferReserveAsset, etc )

Seems a bit more specific then passing the bridge id or destination all the time. And maybe could be useful for other scenarios unrelated to the bridge.

@KiChjang
Copy link
Contributor

KiChjang commented Oct 3, 2023

@serban300 I thought of that too, but in that case, you're just duplicating code for this specific use-case. I would like the solution to be more general than that, hence I suggested putting it into XcmContext altogether.

@serban300
Copy link
Contributor

@KiChjang Not sure if we would need to duplicate code for this. But you might be right. I didn't think of that.

@franciscoaguirre Could I help with this issue ? I would be happy to tackle it if you don't have time. Since we also need it for the bridges.

@serban300
Copy link
Contributor

Published #2021 for this.

@KiChjang in the end I added the FeeReason to handle_fee() since we needed it in order to add separate logic only for ExportMessage and I think in general it should offer more flexibility regarding fee handling. And there was no need for code duplication. But if you have strong preferences towards adding the network ID in the context, please let me know.

@serban300
Copy link
Contributor

Resolved in by #2021

bkchr pushed a commit that referenced this issue Apr 10, 2024
* Make RelayStrategy::final_decision() sync

Signed-off-by: Serban Iorga <serban@parity.io>

* Move logic from RelayStrategy to RelayReference

Signed-off-by: Serban Iorga <serban@parity.io>

* Rename RelayStrategy::final_decision()

Signed-off-by: Serban Iorga <serban@parity.io>
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

No branches or pull requests

3 participants