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

Umbra.sol smart account compatibility #579

Open
mozrt2 opened this issue Jul 27, 2023 · 3 comments
Open

Umbra.sol smart account compatibility #579

mozrt2 opened this issue Jul 27, 2023 · 3 comments

Comments

@mozrt2
Copy link
Contributor

mozrt2 commented Jul 27, 2023

Hi there,

While working on the Stealth Safes POC over the weekend, we initially intended to use the deployed Umbra.sol contracts to send stealth payments and emit corresponding events.

We however ended up having to deploy a modified version of the original contract with one specific change to make it compatible with smart contract based accounts.

The current implementation of sendEth() in Umbra.sol uses _receiver.transfer(amount) to transfer a payment to a stealth address. Since EIP-2929, gas is limited to 2300 when calling .transfer(). This causes an out of gas error when trying to send a payment to a Safe account via Umbra.sol.

EIP-2930 solves this with access lists, as shown here by the Safe team. While using access lists would solve the issue we are facing, they are unfortunately not widely supported by wallets (e.g. see here for Metamask).

To build a working POC, we therefore decided to modify the contract and replace _receiver.transfer(amount) with _receiver.call{value: amount}("") which does not face gas constraints.

It seems unlikely that we find a way to use access lists compatible with all major wallets based on our research.

We therefore wanted to open this issue to discuss potential ways forward. We see two, with the first being the preferred path:

  • Upgrading the current Umbra.sol contract with a new function, sendEthToSmartContract(), where we insert the .call() method and use a reentrancy guard protection
  • Using a separate, modified version of Umbra.sol with these changes for smart accounts
@70nyIT
Copy link

70nyIT commented Jul 27, 2023

If I have to set a preferences over the two, I'd vote on the first option, but as @mozrt2 said, I'd love to hear Umbra team thoughts on this

@mds1
Copy link
Collaborator

mds1 commented Jul 27, 2023

While using access lists would solve the issue we are facing, they are unfortunately not widely supported by wallets (e.g. MetaMask/metamask-extension#11863).

Huh, I'm surprised metamask still doesn't support access lists

To build a working POC, we therefore decided to modify the contract and replace _receiver.transfer(amount) with _receiver.call{value: amount}("") which does not face gas constraints.

This makes sense. I don't remember why we went with .transfer, but I agree it would have been better to forward all gas. I'm guessing we didn't envision a stealth smart contract recipient when the original contract was written a few years ago, and figured it was safer to just forward less gas.

Either way, as you know we're currently working on ERC-5564 and ERC-6538, and will use those contracts as the foundation for the next version of Umbra. @apbendi let me know if you disagree, but I'd prefer not to release an intermediary contract prior to the ERC that just changes the current version's .transfer to .call. This would introduce a lot of work—the subgraph, umbra-js, and frontend all would have to be updated to support both contracts—for just a few months of usage, and it would delay the ERC going live.

Given that, I think the answer for your situation depends on your timeline, i.e. you can either wait for the ERC to be live and leverage the same contracts we build on top, or start with your forked version and pivot to the ERC version down the road.

If you want to DM me your telegram handles, we'd love to chat more about your use case and what you're working on :)

@mozrt2
Copy link
Contributor Author

mozrt2 commented Jul 28, 2023

Thanks for the feedback - this makes total sense & great to hear you are aligned with fixing this in a future contract version - I'll DM now so we can further discuss on Telegram :)

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