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 simple fee mechanism to GMP precompile #2428

Merged
merged 27 commits into from
Aug 29, 2023
Merged

Conversation

notlesh
Copy link
Contributor

@notlesh notlesh commented Aug 9, 2023

What does it do?

Adds a very simple fee mechanism to the GMP precompile. Replaces the much more complicated fee system introduced in #2324.

The added fee mechanism comes in the form of a new VersionedUserAction::V2 which is the same as the previous but includes a fee: U256 field. The fee is paid to the caller of the precompile, which is intended to be the txn sender, but doesn't have to be.

⚠️ Breaking Changes ⚠️

  • All changes are backwards compatible (so not really breaking...?)
  • Adds a new VersionedUserAction variant (V2) which supports a fee
  • The old V1 variant continues to work as it did

Design

The fee is specified in the payload itself. The fee is arbitrary from the protocol's perspective; no one enforces any particular minimum fee. If a fee is too low (or 0) then it is likely that no one will want to relay this message for the user. However, the user is free to relay it for themselves in any case.

I expect relayers who wish to collect fees will have some set minimum they are willing to accept as payment, and anything under this they will not participate as a relay for.

Likewise, I expect dapp UIs to guide users on what an acceptable fee might be, similar to the way wallets do so today.

TODO:

@notlesh notlesh mentioned this pull request Aug 9, 2023
@notlesh notlesh added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. not-breaking Does not need to be mentioned in breaking changes labels Aug 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

Coverage generated "Mon Aug 28 18:11:43 UTC 2023":
https://d3ifz9vhxc2wtb.cloudfront.net/pulls/2428/html/index.html

Master coverage: 87.39%
Pull coverage:

@librelois
Copy link
Collaborator

I suggest to add a fee_beneficiary field in this V2, it's more flexible.
On useful use case if it's the relayer operator want to receive it's reward on a cold wallet, but it can probably be useful in other cases too.

@notlesh
Copy link
Contributor Author

notlesh commented Aug 17, 2023

I suggest to add a fee_beneficiary field in this V2, it's more flexible. On useful use case if it's the relayer operator want to receive it's reward on a cold wallet, but it can probably be useful in other cases too.

Hmmm, that makes sense to let the relayer control the fee beneficiary, but that would be outside the scope of the payload.

Actors:
user: the one bridging something, which includes creating the payload
guardians: the ones signing the VAA + payload
relayer: the one paying for destination-chain (Moonbeam) fees and submitting txn/VAA/payload

So your suggestion makes sense, but doesn't fit in the payload -- I think we would need to make a separate extrinsic (something obnoxious like wormholeTransferERC20WithFeeBeneficiary) in order to accommodate this.

@librelois
Copy link
Collaborator

think we would need to make a separate extrinsic (something obnoxious like wormholeTransferERC20WithFeeBeneficiary) in order to accommodate this.

Ok I see, I think it should not be an extrinsic, but just a precompile selector that take the inner evm call as a parameter

@notlesh
Copy link
Contributor Author

notlesh commented Aug 17, 2023

Ok I see, I think it should not be an extrinsic, but just a precompile selector that take the inner evm call as a parameter

Sorry, to be clear, there is no extrinsic here (there is no pallet and so no pallet calls). You're right, it would be a different precompile function callable from SC.

@librelois librelois mentioned this pull request Aug 25, 2023
28 tasks
@notlesh notlesh merged commit ccbc82c into master Aug 29, 2023
30 checks passed
@notlesh notlesh deleted the notlesh-gmp-simple-fees branch August 29, 2023 13:40
@albertov19
Copy link
Contributor

I know this got merged, but would it make sense to set a maximum in the fee? This could be controlled via governance or something. Just thinking that building the payload is quite complex and the user could be tricked into a fee that for him would be extremely difficult to verify when signing the tx. So capping it to an arbitrary value could be helpful to avoid any misusage of this feature.

WDYT? Sorry that I'm just providing feature right now 😞

@notlesh
Copy link
Contributor Author

notlesh commented Aug 30, 2023

I know this got merged, but would it make sense to set a maximum in the fee? This could be controlled via governance or something. Just thinking that building the payload is quite complex and the user could be tricked into a fee that for him would be extremely difficult to verify when signing the tx. So capping it to an arbitrary value could be helpful to avoid any misusage of this feature.

WDYT? Sorry that I'm just providing feature right now 😞

Hmmm, I can see why it might be helpful, but it would be a bit tricky. For example, should the maximum be enforced when the message is created or when it's processed through the precompile? (The latter is the only one that seems feasible to me, but isn't a good UX).

A feature like this would come with a perf hit, and would be complicated by the fact that the fee would need to be per-asset.

Also, a note on tricking a user: if someone can trick a user like this, they could do far worse than make them specify a high fee -- more likely they'd get away with stealing their funds by changing the amount and recipient :)

@albertov19
Copy link
Contributor

Also, a note on tricking a user: if someone can trick a user like this, they could do far worse than make them specify a high fee -- more likely they'd get away with stealing their funds by changing the amount and recipient :)

Yeah agreed actually this is a great point, at the end of the day you are already trusting the dApp front-end etc and so on because verifying what you are signing is never straightforward.

@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants