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

Fix dynamic fee handling for generic messages #137

Closed
2 tasks
MakMuftic opened this issue Feb 23, 2023 · 0 comments · Fixed by #141
Closed
2 tasks

Fix dynamic fee handling for generic messages #137

MakMuftic opened this issue Feb 23, 2023 · 0 comments · Fixed by #141

Comments

@MakMuftic
Copy link
Contributor

Our current DynamicGenericFeeHandlerEVM.sol implementation of dynamic fee handling for generic messages would not work properly if deployed.

Logic inside function for calculating fee fetches handler based on resourceID and assumes it is compatible IERCHandler interface. This will never be true, as this will always be PermissionlessGenericHandler - that doesn't have function _resourceIDToTokenContractAddress.

This is tight to a bigger problem of DynamicFeeHandler assuming that fee is being paid in token that is transferred - this is not applicable for generic messages. What we actually want is to collect fees in base (native) tokens when executing generic calls.

Implementation details

These are just suggestions and final solution requires deeper dive into solidity code:

  • We need to add some logic inside DynamicFeeHandler that will decide if the fee is being paid in base (native) token or some erc20 token.
  • In conjunction with this, the implementation of _calculateFee inside DynamicGenericFeeHandlerEVM.sol needs to be changed to return 0x00..00 address or something like this - this information will be used to decide DynamicFeeHandler as mentioned in the first point.

Testing details

Revisit current tests, as they were written with wrong presumptions, so tests did not catch this.

  • Add unit tests

Acceptance Criteria

  • Passing unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants