Send native token max without dust

[davidlaprade]

Fixes #378

[netlify[bot]]

✅ Deploy Preview for jolly-shaw-20fe62 ready!

Name	Link
🔨 Latest commit	1e088d3690e501294c0937a0766fe8f4f6df49d1
🔍 Latest deploy log	https://app.netlify.com/sites/jolly-shaw-20fe62/deploys/64136c83e501a900088de284
😎 Deploy Preview	https://deploy-preview-390--jolly-shaw-20fe62.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[davidlaprade]

Test failure does not appear to be related to this PR. It's also failing on master

[garyghayrat]

Mainnet max estimate seems a bit high. Not enough eth left to pay for market gas price.

[davidlaprade]

Mainnet max estimate seems a bit high

Interesting. I'm really glad you checked on mainnet! I actually hadn't. (I needed to republish my keys and I was too lazy/cheap to do it before now. 😅) But since I fixed my mainnet setup, I'm actually seeing the opposite -- estimates are too low:

What fee estimate are you seeing in metamask on mainnet @garyghayrat? This is what I'm currently seeing (on 9/12/22 ~3:45pm ET):

I added some logging and it seems like our mainnet estimates are actually consistently 20% too low, even with the 10% scale factor applied. I'm not sure why that is.

Metamask's estimated gas limit is ~64k for a mainnet sendETH:

And ours is 70671 -- exactly 10% higher than metamask's (as expected, given the scale factor). So the issue seems to be with gas prices.

Sure enough, the MM max gas fee is ~11% higher than what we're getting from our provider. And based on https://www.blocknative.com/gas-estimator theirs is correct. I'll need to look into this

[davidlaprade]

@apbendi I was just able to reproduce the error on Arbitrum

I ultimately got it to work (send tx here) by bumping the native send gas limit but tbh I'm not sure why it was a problem to begin with. The gas used by the tx (372,580) was well under the original estimate (461,905). It seems that this might be an issue with metamask? Unclear. But I bumped the scale factor estimateNativeSendGasLimit for arbitrum by an additional 5% (10% total) and that seemed to fix the issue.

I was also able to reproduce on optimism. Optimism fees are just weird, so there was a real fix needed here. I have to get more OETH before I can test that the fix works though.

I wasn't able to reproduce the issue on mainnet. I was able to drain an account without issue. Could you re-confirm you have this problem?

[apbendi]

Hey @davidlaprade, I gave this another look and tested out both Arbitrum and Mainnet. The "allowance" errors on Arbitrum are gone, however now both networks show this message in MetaMask:

This is the error I see when MetaMask is on "Market", which is the default. If I switch it to "Low", I'm able to confirm the transaction. So it seems maybe the price estimate for gas is just a bit lower than what MetaMask ends up using. Is there a way we can bump it up?

[github-actions[bot]]

LCOV of commit 367cf01 during CI #349

Summary coverage rate:
  lines......: 81.4% (506 of 622 lines)
  functions..: 72.5% (79 of 109 functions)
  branches...: 77.8% (260 of 334 branches)

Files changed coverage rate:
                                                       |Lines       |Functions  |Branches    
  Filename                                             |Rate     Num|Rate    Num|Rate     Num
  ===========================================================================================
  umbra-js/src/utils/utils.ts                          |86.9%    160|80.0%    15|72.3%    101

[davidlaprade]

@apbendi I just rebased on top of master and retried max sends on mainnet and arbitrum, per:

I gave this another look and tested out both Arbitrum and Mainnet. The "allowance" errors on Arbitrum are gone, however now both networks show this message in MetaMask

I didn't get any errors. Native token max sends worked without issue on both networks for me.

This is the error I see when MetaMask is on "Market", which is the default. If I switch it to "Low", I'm able to confirm the transaction

This comment makes me wonder what your MetaMask settings are. I can't even see Market vs Low options unless I switch on an experimental feature (see screenshot). But actually even with this feature switched on I still had an arbitrum max send work without issue.

At this point I think it'd be helpful to hop on a call so I can see what you're seeing.

[davidlaprade]

Alright @apbendi @mds1, I think I finally figured out the remaining issues we were having. I just rebased on master (again) and I think this is ready for re-review.

I have now tested:

• native token max sends (all 4 supported networks)
• native token partial sends (all 4 supported networks)
• ERC20 token max sends (polygon)
• ERC20 token partial sends (polygon)
• receive native token (all 4 supported networks)
• receive ERC20 (polygon)
• native token max sends on the release preview (all L2s)

Everything worked!

cc @garyghayrat as there is one new string to translate

[github-actions[bot]]

Coverage after merging send-native-token-max-without-dust-#378 into master will be

86.13%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
contracts-periphery/src
UmbraBatchSend.sol	93.55%	100%	75%	94.74%	85
UniswapWithdrawHook.sol	77.78%	100%	50%	80%	44
umbra-js/src
ethers.ts	74.58%	100%	58.33%	100%
types.ts	50%	100%	0%	100%
umbra-js/src/classes
KeyPair.ts	98.65%	96.49%	100%	100%	31–32
RandomNumber.ts	100%	100%	100%	100%
StealthKeyRegistry.ts	100%	100%	100%	100%
TxHistoryProvider.ts	85.71%	86.67%	66.67%	87.50%	11, 26, 46, 50, 8
Umbra.ts	79.77%	74.19%	76%	83.66%	134–135, 211–212, 212–213, 222–223, 241, 287–290, 352, 360, 360–362, 364, 366, 386, 386, 386–387, 392–393, 396, 399, 415, 432–433, 481, 515, 532, 545–546, 556–557, 580–581, 657, 660, 660, 660, 663, 670, 670, 677, 681, 681, 681–682, 695, 699, 702–703, 709–711, 718, 718, 718–721
umbra-js/src/utils
cns.ts	50%	33.33%	50%	55%	33–36, 40, 40, 40, 40, 40–41, 44–46
constants.ts	100%	100%	100%	100%
ens.ts	38.89%	25%	33.33%	44%	26–27, 35, 56–57, 57, 57–58, 63–64, 66, 70, 70, 70, 70, 70–71, 75–77
utils.ts	80.58%	72.03%	81.25%	86.29%	102, 120–121, 168, 221, 235–236, 236, 236, 259, 261–262, 271, 271, 271–272, 274, 277, 286–287, 331, 331, 331, 351, 380, 382, 387, 399, 399–400, 405, 407–408, 419–423, 429, 431, 473–474, 474, 474–476, 476, 476, 576, 58, 581, 59, 84, 92, 94–96
umbra-js/test
testPrivateKeys.ts	100%	100%	100%	100%
utils.ts	100%	100%	100%	100%