GoodDollar / GoodCollective

Monorepo for GoodCollective (Segmented UBI and Direct Payments Pool)
MIT License
3 stars 1 forks source link

[Bug] Streaming Donation failed - UNPREDICTABLE_GAS_LIMIT #133

Closed decentralauren closed 5 months ago

decentralauren commented 8 months ago

Unable to complete any streaming donation (tried daily and weekly, both using Celo) - receiving "UNPREDICTABLE_GAS_LIMIT" error.

YOU CAN REPRODUCE THE ERROR USING METAMASK AND GOODDOLLAR WALLET!

Steps to reproduce: 1) Connect GoodWallet to GoodCollective 2) Execute donation flow using CELO 3) Confirm tx in GoodWallet 4) comeback to GoodCollective and observe the result

Actual: The TX failed because of an "Unpredictable gas limit"

Expected: The tx was executed without any issues

Attachments:

Private Zenhub Video

Snag_1a62b2c2.png Snag_1a62e6a3.png

Celoscan - https://celoscan.io/tx/0x48acfbfbf786067d828220ed59ed0fef9a9681b8f633c29302586e14bd6ccab0

krisbitney commented 8 months ago

That error is a smart contract issue

L03TJ3 commented 8 months ago

@sirpy could it be starting a stream requires higher base gas?

sirpy commented 8 months ago

@L03TJ3 @krisbitney When on the donation page I see endless calls to uniswap api token list (reported in #145) Please add to the console log the swapdata and uniswap sdk path sent to the supportflowwithswap I'm guessing it is something wrong with the swappath, does the sdk return multiple possible paths? would be good to log them also

krisbitney commented 8 months ago

@L03TJ3 @krisbitney When on the donation page I see endless calls to uniswap api token list (reported in #145) Please add to the console log the swapdata and uniswap sdk path sent to the supportflowwithswap I'm guessing it is something wrong with the swappath, does the sdk return multiple possible paths? would be good to log them also

Please clarify. Logging and performance optimization are not in scope.

krisbitney commented 8 months ago

@L03TJ3 @krisbitney When on the donation page I see endless calls to uniswap api token list (reported in #145) Please add to the console log the swapdata and uniswap sdk path sent to the supportflowwithswap I'm guessing it is something wrong with the swappath, does the sdk return multiple possible paths? would be good to log them also

Okay, I understand what you're saying. The repeated calls are fixed in https://github.com/GoodDollar/GoodCollective/pull/148

krisbitney commented 8 months ago

@sirpy Regarding the swap path, only the optimal swap path is returned by the SDK.

sirpy commented 8 months ago

@krisbitney logging is not required, but from my POV there's something wrong with the swap path. STF exception is when uniswap router can't transferFrom the user the initial token for the swap. Another option is that the approve target is incorrect, which contract is the user approving the token to? it should be the pool not uniswap router. logging is so I can help you debug it, but you can debug it yourself to make sure the swap path is correct or report what it is, so I can verify.

krisbitney commented 8 months ago

@krisbitney logging is not required, but from my POV there's something wrong with the swap path. STF exception is when uniswap router can't transferFrom the user the initial token for the swap. logging is so I can help you debug it, but you can debug it yourself to make sure the swap path is correct or report what it is, so I can verify.

I cannot see the images, so maybe I am missing something.

The UNPREDICTABLE_GAS_LIMIT exception is thrown when the wallet's estimateGas function fails. The causes for it can vary widely, but it typically means there is an issue in the smart contract that prevented the transaction simulation from completing.

sirpy commented 8 months ago

@krisbitney from the image and from my attempt the revert reason is STF as i've described above. if you try it yourself and examine the full console error you will see the STF reason

krisbitney commented 8 months ago

Example route:

...
  ],
  "tokenPath": [
    {
      "chainId": 42220,
      "decimals": 18,
      "symbol": "CELO",
      "isNative": false,
      "isToken": true,
      "address": "0x471EcE3750Da237f93B8E339c536989b8978a438"
    },
    {
      "chainId": 42220,
      "decimals": 18,
      "symbol": "G$",
      "isNative": false,
      "isToken": true,
      "address": "0x62B8B11039FcfE5aB0C56E502b1C372A3d2a9c7A"
    }
  ],
  "input": {
    "chainId": 42220,
    "decimals": 18,
    "symbol": "CELO",
    "isNative": false,
    "isToken": true,
    "address": "0x471EcE3750Da237f93B8E339c536989b8978a438"
  },
  "output": {
    "chainId": 42220,
    "decimals": 18,
    "symbol": "G$",
    "isNative": false,
    "isToken": true,
    "address": "0x62B8B11039FcfE5aB0C56E502b1C372A3d2a9c7A"
  },
  "protocol": "V3"
}

example path:

0x471ece3750da237f93b8e339c536989b8978a43800271062b8b11039fcfe5ab0c56e502b1c372a3d2a9c7a
sirpy commented 8 months ago

@krisbitney The issue seems to be the approve recipient. it needs to be the pool not uniswap router.

krisbitney commented 8 months ago

@krisbitney The issue seems to be the approve recipient. it needs to be the pool not uniswap router.

Okay. With Uniswap, you're typically supposed to approve the Uniswap router since it transfers the funds.

Changing the approve recipient to the pool produces this exception instead:

Screenshot 2024-02-01 at 9 47 34 PM
krisbitney commented 8 months ago

I think that solution also fails to explain why swap donations have worked in the past

decentralauren commented 8 months ago

@krisbitney - what is still needed to resolve this?

L03TJ3 commented 8 months ago

@krisbitney @sirpy what is the status here? Does it need additional work/research?

krisbitney commented 8 months ago

@krisbitney @sirpy what is the status here? Does it need additional work/research?

I will do it today

krisbitney commented 8 months ago

@sirpy I would like to show you that the swap path is correct.

For completeness, I've created a demo branch to show you that I can execute the swaps returned from the Uniswap SDK. In this PR's branch, when you try to donate with CELO, it will first try to do the normal supportFlowWithSwap and then it will execute the same swap directly (without donating). https://github.com/GoodDollar/GoodCollective/pull/156

The path encoding of a route is simple:

input token
pool fee
[intermediate token, pool fee, ...] // for swaps with hops between pools
output token

This can be seen in encodeRouteToPath in the Uniswap SDK here: https://github.com/Uniswap/v3-sdk/blob/main/src/utils/encodeRouteToPath.ts

This is the example path I am passing into the GoodCollective SDK:

0x471ece3750da237f93b8e339c536989b8978a43800271062b8b11039fcfe5ab0c56e502b1c372a3d2a9c7a

// breakdown
471ece3750da237f93b8e339c536989b8978a438 // input token
002710 // 10_000 (pool fee rate)
62b8b11039fcfe5ab0c56e502b1c372a3d2a9c7a // output token

The path corresponds correctly to the route tokens:

      {
        "chainId": 42220,
        "decimals": 18,
        "symbol": "CELO",
        "isNative": false,
        "isToken": true,
        "address": "0x471EcE3750Da237f93B8E339c536989b8978a438"
      },
      {
        "chainId": 42220,
        "decimals": 18,
        "symbol": "G$",
        "isNative": false,
        "isToken": true,
        "address": "0x62B8B11039FcfE5aB0C56E502b1C372A3d2a9c7A"
      }

And the pool fee:

       "fee": 10000,

The pool can be viewed on info.uniswap.org at https://info.uniswap.org/#/celo/pools/0xcb037f27eb3952222810966e28e0ceb650c65cd9

    "poolAddresses": [
      "0xCB037f27eB3952222810966e28E0cEB650c65CD9"
    ],

Here is what I am passing into supportFlowWithSwap:

const tx = await sdk.supportFlowWithSwap(
  signer, 
  "0x11f18e8f2a27d54a605cf10486b3d4c5aeeba81f",  // collective
  "115740740740", // flow rate
  {
    "amount": "10000000000000000", // 0.01 CELO
    "minReturn": "191333558074193378434", // ~191.33 G$
    "path": "0x471ece3750da237f93b8e339c536989b8978a43800271062b8b11039fcfe5ab0c56e502b1c372a3d2a9c7a",
    "swapFrom": "0x471EcE3750Da237f93B8E339c536989b8978a438", // CELO
    "deadline": "1706870449"
  }
);

It is the same, correctly encoded path for the route. Do the other parameters look reasonable to you?

krisbitney commented 8 months ago

@krisbitney - what is still needed to resolve this?

@decentralauren I believe when @sirpy reads the walkthrough above, he will agree the bug is most likely in the smart contracts or GoodCollective JavaScript SDK.

sirpy commented 8 months ago

@krisbitney the approve was a correct fix. but you are right, it seems the celo uniswap router is a new version that doesn't support the deadline param. i'll need to fix the smart contract and redeploy

sirpy commented 8 months ago

@krisbitney there's a new pool with fixed code (Beacon Fixed...) I've tested manually and swapandstream works. the UI still needs the approve fix to work correctly

krisbitney commented 8 months ago

@sirpy Could you tell me more about the issue you're having with approve? Maybe create an issue with reproduction steps? I have never once had an issue with token approval, and this is the first I've heard of anyone having issues with it.

krisbitney commented 8 months ago

@krisbitney the approve was a correct fix. but you are right, it seems the celo uniswap router is a new version that doesn't support the deadline param. i'll need to fix the smart contract and redeploy

@sirpy Could you tell me more about the issue you're having with approve? Maybe create an issue with reproduction steps? I have never once had an issue with token approval, and this is the first I've heard of anyone having issues with it.

Okay, I see what you're saying. You're saying you think we need to approve the pool contract instead of the router. Okay, I will do that soon

krisbitney commented 8 months ago

@sirpy are you aware that I successfully executed swap and stream before everyone started hitting this bug? Do you know what might have changed to cause the bug, or why now we need to approve the pool instead of the uniswap router?

sirpy commented 8 months ago

@krisbitney I doubt it. can you send me the TX where you did that? It could have happened if you previously for some reason approved the token to be swapped to the pool. In order for the swap and stream to happen in 1 tx, it has to happen from a single smart contract - the pool in our case. The contract has to perform the swap, in order to perform the swap the contract MUST get the tokens to swap from the user and then send then to uniswap.

Nothing has changed that i'm aware of, it was liked that from the beginning, yes we need to approve the pool and not the uniswap router.

krisbitney commented 8 months ago

@krisbitney I doubt it. can you send me the TX where you did that? It could have happened if you previously for some reason approved the token to be swapped to the pool. In order for the swap and stream to happen in 1 tx, it has to happen from a single smart contract - the pool in our case. The contract has to perform the swap, in order to perform the swap the contract MUST get the tokens to swap from the user and then send then to uniswap.

Nothing has changed that i'm aware of, it was liked that from the beginning, yes we need to approve the pool and not the uniswap router.

I don't want to argue with you. Believe whatever you want.

I created a PR that changes the address approved: https://github.com/GoodDollar/GoodCollective/pull/163

It doesn't fix the problem.

sirpy commented 8 months ago

its not about arguing, please send the TX so I can debug if what you say is true, its just logically doesnt make sense that it did work, for the reason I explained, so please try to find that TX. Now its a different error, if you have an existing flow it will fail, i'm updating the sdk.

decentralauren commented 8 months ago

@L03TJ3 waiting for your code review then @vldkhh need to retest.

vldkhh commented 8 months ago

@sirpy @krisbitney @decentralauren still reproducing Snag_360f3046

https://explorer.celo.org/mainnet/tx/0xc7e03e18634abf96bb2f679f80bdfb609b14efc79db3afcf9656bb31fc123ef6

@sirpy let me know if you need any additional information or logs from it

sirpy commented 8 months ago

@vldkhh this needs to be tested on the new pools ("new beacon...") The silvi pool contract was not upgraded yet.

vldkhh commented 8 months ago

@sirpy @krisbitney I've tested this flow on Beacon Collective. It worked without failing and I used MM. But I see that the funds reflect on my GD wallet. @sirpy is it the expected result? here tx information: tx hash from GC - https://explorer.celo.org/mainnet/tx/0x4edb65a0a6857bb440248bc739bb1770fc38b79b42efde8a57a99739780403bf

tx from GD wallet - https://celoscan.io/tx/0x4edb65a0a6857bb440248bc739bb1770fc38b79b42efde8a57a99739780403bf

sirpy commented 7 months ago

@vldkhh yes you get the G$ in your wallet, but your G$ wallet balance should decline with time, according to the stream speed you selected

decentralauren commented 7 months ago

@vldkhh does this pass QA?

vldkhh commented 7 months ago

@decentralauren @L03TJ3 this error is related to the amount of donation, if its too low - the tx will fail. Did we handle it somehow? or not yet?

sirpy commented 7 months ago

@decentralauren I suggest we close this as fixed. open a new issue to add some minimum amount

decentralauren commented 7 months ago

@sirpy Yes that is fine, but can you help me understand the issue? I'm not clear on what happened / why a minimum amount is required

sirpy commented 7 months ago

this is more of a superfluid protocol things, that i've enforced in the goodcollective contracts. if a user account runs out of funds the stream has to be closed, since there is no automation on the blockchain, someone has to issue this TX to close the stream. A buffer is taken relative to the size of the stream, this buffer is used to also compensate such closing TXs. To have enough incentive to cover the TX cost the buffer has to be above a certain size.

See Buffers and Sentinels here: https://docs.superfluid.finance/docs/concepts/overview/money-streaming#buffer

github-actions[bot] commented 5 months ago

Stale issue message

decentralauren commented 5 months ago

Created new issue