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

Allow compensation for relayed msg.value ETH #163

Closed
nuevoalex opened this issue Dec 23, 2019 · 6 comments
Closed

Allow compensation for relayed msg.value ETH #163

nuevoalex opened this issue Dec 23, 2019 · 6 comments

Comments

@nuevoalex
Copy link

Context / issue

It would be very useful for the contracts (and the relayer) to support compensation for the tx sender sending the ETH specified by msg.value. This is particularly relevant for wallets where the intention is that they never hold ETH and compensate the tx sender entirely in DAI.

Proposed solution

An additional param provided to the execTransaction call specifying that the msg.value should be included in the handlePayment method. It would simply add the msg.value to the total value of ETH needing to be paid by the wallet to the refundReceiver.

As a separate but related feature the relayer would also need to send along the msg.value in its TX when executing the safe tx.

@rmeissner
Copy link
Member

Hey,

thanks for the proposal. Currently I would not make execTransaction payable. A workaround would be to use MultiSend to convert some of your DAI to ETH during the transaction (e.g. via Kyber or Uniswap).

@joeykrug
Copy link

joeykrug commented Jan 7, 2020

What's multisend?

@rmeissner
Copy link
Member

https://github.com/gnosis/safe-contracts/blob/development/contracts/libraries/MultiSend.sol (tests: https://github.com/gnosis/safe-contracts/blob/development/test/multiSend.js)

This is a contract that can be used via a delegatecall and allows you do batch multiple transactions. Obviously you could also just create a simple contract that does what you need explicitly and use it via delegatecall.

Adding this refund to the core contract is a little out of scope (in my opinion). Assuming the user wants to refund the ETH in DAI, then we need to somehow provide a conversion rate between ETH and DAI. Additionally we need to define what happens if the internal transaction (that should use the ETH send along) fails and therefore the ETH is left on the Safe (which is undesired if you have a DAI only Safe). So there are quite some edge cases which need to be properly defined.

@joeykrug
Copy link

joeykrug commented Jan 9, 2020

Hmm, we already have to convert between dai and Eth anytime a user pays someone’s gas, what’s different here?

@rmeissner
Copy link
Member

So the gasPrice parameter is a conversion rate from your gasToken (e.g. DAI) to the ETH gasPrice (not the ETH price) ... that conversion rate can not be used to convert ETH to DAI, also this price is not 100% related to tx.gasprice, so if we would try to use this to calculate the ETH-DAI conversion rate the relayer could influence it.

@rmeissner
Copy link
Member

Will not be fixed as we plan to remove the payment logic in the next major version: #197

fdarian pushed a commit to fdarian/safe-contracts that referenced this issue Jan 14, 2024
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