Bump uniswap sdk core to 4.09 and add correct base swap router address #481

sismanis · 2024-01-29T23:53:10Z

The swap router address for base is different for other chains so the function needs to account for that. Also, @uniswap/sdk-core needs to be bumped to 4.0.9 where the WETH9 contract for Base was added otherwise any pools with WETH on base will not work in this SDK with the swap router.

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
This will fix bugs with using base with the smart order router. Currently does not work with the current version of the sdk and is fixed with these changes
What is the current behavior? (You can also link to an open issue here)
The current behavior will throw an error when requesting a swap router quote on base
What is the new behavior (if this is a feature change)?
The swap router will work correctly and quotes are able to be fetched using the SDK
Other information:

wminshew · 2024-02-06T23:14:18Z

would be great to get this merged, currently using the smart-order-router on base results in a loss of funds since they're sent to an EOA the function call "succeeds"

cgkol · 2024-02-06T23:25:07Z

hey all,

We are currently not supporting actively the swapRouter02 and are planning on deprecating it soon. Instead, we recommend using the UniversalRouter which should have everything configured as expected.

wminshew · 2024-02-07T17:53:53Z

hey all,

We are currently not supporting actively the swapRouter02 and are planning on deprecating it soon. Instead, we recommend using the UniversalRouter which should have everything configured as expected.

it looks like the address in the docs doesn't match the address being used in the universal-router-sdk either. I think the docs are wrong and the sdk is right, but would you confirm for me? Obviously a little extra nervous here given we've already lost funds with the last wrong address

cgkol · 2024-02-08T15:46:14Z

@wminshew thank you for the flag. Before I give you a final confirmation, let me circle back to confirm the right addresses and will get back to you asap

hensha256 · 2024-02-08T17:40:46Z

hey @sismanis , thanks so much for bringing this to our attention. I've dug further and it looks like we're facing the same issue with Avalanche too. Would you mind adding a further conditional for avalanche too please in your PR?
You can see here that avalanche has this address

hensha256 · 2024-02-08T18:07:48Z

it looks like the address in the docs doesn't match the address being used in the universal-router-sdk either. I think the docs are wrong and the sdk is right, but would you confirm for me? Obviously a little extra nervous here given we've already lost funds with the last wrong address

Sure! The address for universal-router-sdk is different because we deployed a new one in the last couple of weeks to add Uniswap V2 support (it previously only had Uniswap V3 supported on Base.). We have not added that router to our interface yet, so we are still working with 0x198EF79F1F515F02dFE9e3115eD9fC07183f02fC which you can find in our docs and in old universal-router-sdk versions! If youre only using Uniswap V3 then the address 0x198... is still perfect to use, or if you want Uniswap V2 support too, you should use the address in the latest SDK version.

hensha256 · 2024-02-08T19:09:20Z

I've made this PR for sdk-core, I think we should merge that as a catch-all to make sure this never happens again. Then we can publish that and you can integrate it here. What do you think?

jsy1218 · 2024-02-09T06:59:53Z

Personally I prefer to separate support OPTIMISM_SEPOLIA from this PR. Previously there was a PR about supporting OPTIMISM_SEPOLIA.

sismanis · 2024-02-09T17:05:56Z

hey @sismanis , thanks so much for bringing this to our attention. I've dug further and it looks like we're facing the same issue with Avalanche too. Would you mind adding a further conditional for avalanche too please in your PR? You can see here that avalanche has this address

Hey @hensha256 just added Avalanche address as well. I'm assuming it's in the SDK-core like the Base one is as well. If not can hardcode it in, but probably better to add it into core directly and reference

hensha256 · 2024-02-13T20:38:07Z

Hey! The PR I talked about in this message has now been merged and published into sdk-core version 4.1.2. If you want to update to that version, you should be able to update the PR to just use that function without special casing :)

hensha256 · 2024-02-13T20:39:12Z

Would also be great if you could separate the sepolia stuff into another PR as @jsy1218 said!

jsy1218 · 2024-02-14T03:11:54Z

Would also be great if you could separate the sepolia stuff into another PR as @jsy1218 said!

@sismanis I already merged OP Sepolia support. You should be able to separate just by merging from main branch.

sismanis · 2024-02-14T14:56:38Z

@jsy1218 sounds good will do that today

sismanis · 2024-02-14T15:26:15Z

@jsy1218 I can't update to sdk-core version 4.1.2 without adding arbitrum-sepolia support, and I need 4.1.2 to get the new swap router addresses exported from sdk-core. Do you want me to do that in a separate PR or do it here as well.

jsy1218 · 2024-02-15T02:29:06Z

@jsy1218 I can't update to sdk-core version 4.1.2 without adding arbitrum-sepolia support, and I need 4.1.2 to get the new swap router addresses exported from sdk-core. Do you want me to do that in a separate PR or do it here as well.

Sorry for the trouble, but would love to support arb-sepolia in a separate PR.

jsy1218 · 2024-02-15T13:27:50Z

@sismanis FYI I created the Arb-Sepolia PR. This PR has been hanging for a bit, and continuous user fund loss is a real risk. If possible, I'd like to lend a help and deploy to routing-api Thursday during EST business hours.

sismanis · 2024-02-15T14:08:05Z

@sismanis FYI I created the Arb-Sepolia PR. This PR has been hanging for a bit, and continuous user fund loss is a real risk. If possible, I'd like to lend a help and deploy to routing-api Thursday during EST business hours.

Let me know when the arb PR is merged and I can rebase off of main and finalize the changes. Sure, I'm available pretty much any time today

jsy1218 · 2024-02-15T17:45:09Z

@sismanis FYI I created the Arb-Sepolia PR. This PR has been hanging for a bit, and continuous user fund loss is a real risk. If possible, I'd like to lend a help and deploy to routing-api Thursday during EST business hours.

Let me know when the arb PR is merged and I can rebase off of main and finalize the changes. Sure, I'm available pretty much any time today

@sismanis just merged :)

package.json

sismanis · 2024-02-15T17:55:45Z

Will reopen in a sec, just found it easier to reset to main and reimplement cause I had a lot of similar/conflicting changes with the sepolia stuff

sismanis · 2024-02-15T18:02:57Z

Should be good to go now @jsy1218. I noticed some conlficting keys in a test case with the new sepolia chains. Can fix that real quick in this PR if you want too

jsy1218 · 2024-02-15T18:06:41Z

Should be good to go now @jsy1218. I noticed some conlficting keys in a test case with the new sepolia chains. Can fix that real quick in this PR if you want too

That's fine, you can leave as is. I think the duplicate key is fine, since other PR can have all the tests pass #493.

jsy1218 · 2024-02-15T18:10:20Z

Integration Tests will fail, if the branch is forked from SOR. This is because a bunch of secrets used in https://github.com/Uniswap/smart-order-router/blob/main/.github/workflows/tests.yml require creating new repository secrets:

      JSON_RPC_PROVIDER: ${{ secrets.JSON_RPC_PROVIDER }}

      JSON_RPC_PROVIDER_GORLI: ${{ secrets.JSON_RPC_PROVIDER_GORLI }}

      JSON_RPC_PROVIDER_OPTIMISM: ${{ secrets.JSON_RPC_PROVIDER_OPTIMISM }}

      JSON_RPC_PROVIDER_OPTIMISM_GOERLI: ${{ secrets.JSON_RPC_PROVIDER_OPTIMISM_GOERLI }}

      JSON_RPC_PROVIDER_ARBITRUM_ONE: ${{ secrets.JSON_RPC_PROVIDER_ARBITRUM_ONE }}

      JSON_RPC_PROVIDER_ARBITRUM_GOERLI: ${{ secrets.JSON_RPC_PROVIDER_ARBITRUM_GOERLI }}

      JSON_RPC_PROVIDER_POLYGON: ${{ secrets.JSON_RPC_PROVIDER_POLYGON }}

      JSON_RPC_PROVIDER_POLYGON_MUMBAI: ${{ secrets.JSON_RPC_PROVIDER_POLYGON_MUMBAI }}

      JSON_RPC_PROVIDER_CELO: ${{ secrets.JSON_RPC_PROVIDER_CELO }}

      JSON_RPC_PROVIDER_CELO_ALFAJORES: ${{ secrets.JSON_RPC_PROVIDER_CELO_ALFAJORES }}

      JSON_RPC_PROVIDER_BNB: ${{ secrets.JSON_RPC_PROVIDER_BNB }}

      JSON_RPC_PROVIDER_AVALANCHE: ${{ secrets.JSON_RPC_PROVIDER_AVALANCHE }}

      JSON_RPC_PROVIDER_BASE: ${{ secrets.JSON_RPC_PROVIDER_BASE }}

      TENDERLY_BASE_URL: ${{ secrets.TENDERLY_BASE_URL }}

      TENDERLY_USER: ${{ secrets.TENDERLY_USER }}

      TENDERLY_PROJECT: ${{ secrets.TENDERLY_PROJECT }}

      TENDERLY_ACCESS_KEY: ${{ secrets.TENDERLY_ACCESS_KEY }}

The changes seems ok to me, and I'm ok merging it first, and see the main branch can have all the tests pass.

jsy1218 · 2024-02-15T18:16:34Z

Integration Tests will fail, if the branch is forked from SOR. This is because a bunch of secrets used in https://github.com/Uniswap/smart-order-router/blob/main/.github/workflows/tests.yml require creating new repository secrets:

      JSON_RPC_PROVIDER: ${{ secrets.JSON_RPC_PROVIDER }}

      JSON_RPC_PROVIDER_GORLI: ${{ secrets.JSON_RPC_PROVIDER_GORLI }}

      JSON_RPC_PROVIDER_OPTIMISM: ${{ secrets.JSON_RPC_PROVIDER_OPTIMISM }}

      JSON_RPC_PROVIDER_OPTIMISM_GOERLI: ${{ secrets.JSON_RPC_PROVIDER_OPTIMISM_GOERLI }}

      JSON_RPC_PROVIDER_ARBITRUM_ONE: ${{ secrets.JSON_RPC_PROVIDER_ARBITRUM_ONE }}

      JSON_RPC_PROVIDER_ARBITRUM_GOERLI: ${{ secrets.JSON_RPC_PROVIDER_ARBITRUM_GOERLI }}

      JSON_RPC_PROVIDER_POLYGON: ${{ secrets.JSON_RPC_PROVIDER_POLYGON }}

      JSON_RPC_PROVIDER_POLYGON_MUMBAI: ${{ secrets.JSON_RPC_PROVIDER_POLYGON_MUMBAI }}

      JSON_RPC_PROVIDER_CELO: ${{ secrets.JSON_RPC_PROVIDER_CELO }}

      JSON_RPC_PROVIDER_CELO_ALFAJORES: ${{ secrets.JSON_RPC_PROVIDER_CELO_ALFAJORES }}

      JSON_RPC_PROVIDER_BNB: ${{ secrets.JSON_RPC_PROVIDER_BNB }}

      JSON_RPC_PROVIDER_AVALANCHE: ${{ secrets.JSON_RPC_PROVIDER_AVALANCHE }}

      JSON_RPC_PROVIDER_BASE: ${{ secrets.JSON_RPC_PROVIDER_BASE }}

      TENDERLY_BASE_URL: ${{ secrets.TENDERLY_BASE_URL }}

      TENDERLY_USER: ${{ secrets.TENDERLY_USER }}

      TENDERLY_PROJECT: ${{ secrets.TENDERLY_PROJECT }}

      TENDERLY_ACCESS_KEY: ${{ secrets.TENDERLY_ACCESS_KEY }}

The changes seems ok to me, and I'm ok merging it first, and see the main branch can have all the tests pass.

I noticed that Tests / Unit Tests (pull_request) failed, which is set to be required for the branch protection rule, and block this PR from merging. For it to pass, it requires setting JSON_RPC_PROVIDER to your forked repo https://github.com/L1-Advisors/smart-order-router/tree/fix-base-swap-router. Do you have a JSON PRC ready? A free API key from Infura should do, i.e. https://mainnet.infura.io/v3/{API_KEY}

sismanis · 2024-02-15T18:19:37Z

Integration Tests will fail, if the branch is forked from SOR. This is because a bunch of secrets used in https://github.com/Uniswap/smart-order-router/blob/main/.github/workflows/tests.yml require creating new repository secrets:
      JSON_RPC_PROVIDER: ${{ secrets.JSON_RPC_PROVIDER }}
      JSON_RPC_PROVIDER_GORLI: ${{ secrets.JSON_RPC_PROVIDER_GORLI }}
      JSON_RPC_PROVIDER_OPTIMISM: ${{ secrets.JSON_RPC_PROVIDER_OPTIMISM }}
      JSON_RPC_PROVIDER_OPTIMISM_GOERLI: ${{ secrets.JSON_RPC_PROVIDER_OPTIMISM_GOERLI }}
      JSON_RPC_PROVIDER_ARBITRUM_ONE: ${{ secrets.JSON_RPC_PROVIDER_ARBITRUM_ONE }}
      JSON_RPC_PROVIDER_ARBITRUM_GOERLI: ${{ secrets.JSON_RPC_PROVIDER_ARBITRUM_GOERLI }}
      JSON_RPC_PROVIDER_POLYGON: ${{ secrets.JSON_RPC_PROVIDER_POLYGON }}
      JSON_RPC_PROVIDER_POLYGON_MUMBAI: ${{ secrets.JSON_RPC_PROVIDER_POLYGON_MUMBAI }}
      JSON_RPC_PROVIDER_CELO: ${{ secrets.JSON_RPC_PROVIDER_CELO }}
      JSON_RPC_PROVIDER_CELO_ALFAJORES: ${{ secrets.JSON_RPC_PROVIDER_CELO_ALFAJORES }}
      JSON_RPC_PROVIDER_BNB: ${{ secrets.JSON_RPC_PROVIDER_BNB }}
      JSON_RPC_PROVIDER_AVALANCHE: ${{ secrets.JSON_RPC_PROVIDER_AVALANCHE }}
      JSON_RPC_PROVIDER_BASE: ${{ secrets.JSON_RPC_PROVIDER_BASE }}
      TENDERLY_BASE_URL: ${{ secrets.TENDERLY_BASE_URL }}
      TENDERLY_USER: ${{ secrets.TENDERLY_USER }}
      TENDERLY_PROJECT: ${{ secrets.TENDERLY_PROJECT }}
      TENDERLY_ACCESS_KEY: ${{ secrets.TENDERLY_ACCESS_KEY }}
The changes seems ok to me, and I'm ok merging it first, and see the main branch can have all the tests pass.
I noticed that Tests / Unit Tests (pull_request) failed, which is required, and block the merge. For it to pass, it requires setting JSON_RPC_PROVIDER to your forked repo https://github.com/L1-Advisors/smart-order-router/tree/fix-base-swap-router. Do you have a JSON PRC ready? A free API key from Infura should do, i.e. https://mainnet.infura.io/v3/{API_KEY}

Yeah no problem I have one ready. Will set that now

sismanis · 2024-02-15T18:25:17Z

Alright @jsy1218 just added JSON_RPC_PROVIDER to my repo secrets. Should be good to go

jsy1218 · 2024-02-15T18:31:47Z

Alright @jsy1218 just added JSON_RPC_PROVIDER to my repo secrets. Should be good to go

Weird, I manually retried unit test, and the latest run shows that JSON_RPC_PROVIDER is still empty:

Did you add as Repository Secrets?

sismanis · 2024-02-15T18:34:25Z

Alright @jsy1218 just added JSON_RPC_PROVIDER to my repo secrets. Should be good to go

Weird, I manually retried unit test, and the latest run shows that JSON_RPC_PROVIDER is still empty:
Did you add as Repository Secrets?

Yep, that's where I added it. Very weird.

sismanis · 2024-02-15T18:37:00Z

@jsy1218 just updated it again and double checked it was there idk if I messed something up the first time

jsy1218 · 2024-02-15T18:42:42Z

@jsy1218 just updated it again and double checked it was there idk if I messed something up the first time

Manully retryed again. It fails again, and I can't quite tell why. I think I will create a PR within Uniswap SOR, so as to ship this fix asap.

sismanis · 2024-02-15T18:49:50Z

@jsy1218 just updated it again and double checked it was there idk if I messed something up the first time

Manully retryed again. It fails again, and I can't quite tell why. I think I will create a PR within Uniswap SOR, so as to ship this fix asap.

That's a shame I was hoping to get some contribution credit to the repo. Would it work if I created the PR within this repo and not from a fork? I can handle it if that's the case

jsy1218 · 2024-02-15T18:56:24Z

@jsy1218 just updated it again and double checked it was there idk if I messed something up the first time

Manully retryed again. It fails again, and I can't quite tell why. I think I will create a PR within Uniswap SOR, so as to ship this fix asap.

That's a shame I was hoping to get some contribution credit to the repo. Would it work if I created the PR within this repo and not from a fork? I can handle it if that's the case

Well.. Only Uniswap Labs employees can be added to the Uniswap org, in order to create the PR within https://github.com/Uniswap repos.

Can you disable all tests in token-properties-provider.test.ts and token-fee-fetcher.test.ts? That's probably gonna make unit test pass as well. I think I will follow up by putting token-properties-provider.test.ts and token-fee-fetcher.test.ts out of the unit test suite. I tested locally, disable all tests in token-properties-provider.test.ts and token-fee-fetcher.test.ts can make unit test pass.

This way you can still get contribution credit, meanwhile the repo can be better structured to serve future community contributions as well.

sismanis · 2024-02-15T20:11:43Z

@jsy1218 just updated it again and double checked it was there idk if I messed something up the first time

Manully retryed again. It fails again, and I can't quite tell why. I think I will create a PR within Uniswap SOR, so as to ship this fix asap.

That's a shame I was hoping to get some contribution credit to the repo. Would it work if I created the PR within this repo and not from a fork? I can handle it if that's the case

Well.. Only Uniswap Labs employees can be added to the Uniswap org, in order to create the PR within https://github.com/Uniswap repos.

Can you disable all tests in token-properties-provider.test.ts and token-fee-fetcher.test.ts? That's probably gonna make unit test pass as well. I think I will follow up by putting token-properties-provider.test.ts and token-fee-fetcher.test.ts out of the unit test suite. I tested locally, disable all tests in token-properties-provider.test.ts and token-fee-fetcher.test.ts can make unit test pass.

This way you can still get contribution credit, meanwhile the repo can be better structured to serve future community contributions as well.

Yep, I will do that right now. Thanks so much.

sismanis · 2024-02-15T21:38:04Z

@jsy1218 just FYI I skipped the tests in those files. Not 100% sure if I did it correctly.

jsy1218 · 2024-02-16T12:25:38Z

@jsy1218 just FYI I skipped the tests in those files. Not 100% sure if I did it correctly.

@sismanis I think you can merge

sismanis · 2024-02-16T16:32:46Z

@jsy1218 just FYI I skipped the tests in those files. Not 100% sure if I did it correctly.

@sismanis I think you can merge

I can't merge, says only those with write access can merge to this repository

jsy1218 · 2024-02-18T11:46:48Z

@jsy1218 just FYI I skipped the tests in those files. Not 100% sure if I did it correctly.

@sismanis I think you can merge

I can't merge, says only those with write access can merge to this repository

Ok, I can help merge next Tuesday EST time. Monday is a company holiday.

…r address (#481)" This reverts commit a9f037a.

* Bump V2-SDK to 4.2.0 * 3.24.1 * 3.24.2 * fix test * update dependencies * Revert "Bump uniswap sdk core to 4.09 and add correct base swap router address (#481)" This reverts commit a9f037a. * increase gas deviation

jsy1218 · 2024-02-23T00:04:55Z

@sismanis FYI, ~~this PR was reverted due to regression. I haven't had the time to figure out what's wrong with the PR yet.~~ How much user fund loss risk are we talking about here?

Update - the regression might be explained here Uniswap/sdk-core#120

sismanis requested a review from a team as a code owner January 29, 2024 23:53

jsy1218 mentioned this pull request Feb 9, 2024

feat: support op sepolia #486

Merged

jsy1218 mentioned this pull request Feb 15, 2024

feat: support arb-sepolia #493

Merged

jsy1218 reviewed Feb 15, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

sismanis closed this Feb 15, 2024

sismanis force-pushed the fix-base-swap-router branch from 737df46 to f8dd7b3 Compare February 15, 2024 17:54

Finalize swap router changes

b555563

sismanis reopened this Feb 15, 2024

jsy1218 approved these changes Feb 15, 2024

View reviewed changes

Skipping unit tests in token-properties-provider and token-fee-fetcher

b281124

jsy1218 approved these changes Feb 16, 2024

View reviewed changes

jsy1218 merged commit a9f037a into Uniswap:main Feb 20, 2024
8 of 23 checks passed

This was referenced Feb 20, 2024

chore: bump to v3.24.1 #496

Merged

chore: bump sor to 3.24.1 Uniswap/routing-api#472

Closed

mikeki added a commit that referenced this pull request Feb 22, 2024

Revert "Bump uniswap sdk core to 4.09 and add correct base swap route…

52d81c7

…r address (#481)" This reverts commit a9f037a.

jsy1218 mentioned this pull request Feb 22, 2024

Bump v2 SDK and increase SOR version #499

Merged

This was referenced Feb 23, 2024

Fixed bug that returned an empty swap router 02 address for base chain Uniswap/sdk-core#120

Merged

fix: bump sdk-core and v2-sdk and correct base swap router address #505

Merged

Bump uniswap sdk core to 4.09 and add correct base swap router address #481

Bump uniswap sdk core to 4.09 and add correct base swap router address #481

Conversation

sismanis commented Jan 29, 2024

wminshew commented Feb 6, 2024

cgkol commented Feb 6, 2024

wminshew commented Feb 7, 2024

cgkol commented Feb 8, 2024

hensha256 commented Feb 8, 2024

hensha256 commented Feb 8, 2024

hensha256 commented Feb 8, 2024

jsy1218 commented Feb 9, 2024 • edited Loading

sismanis commented Feb 9, 2024

hensha256 commented Feb 13, 2024

hensha256 commented Feb 13, 2024

jsy1218 commented Feb 14, 2024

sismanis commented Feb 14, 2024

sismanis commented Feb 14, 2024

jsy1218 commented Feb 15, 2024

jsy1218 commented Feb 15, 2024

sismanis commented Feb 15, 2024

jsy1218 commented Feb 15, 2024

sismanis commented Feb 15, 2024

sismanis commented Feb 15, 2024

jsy1218 commented Feb 15, 2024

jsy1218 commented Feb 15, 2024

jsy1218 commented Feb 15, 2024 • edited Loading

sismanis commented Feb 15, 2024

sismanis commented Feb 15, 2024

jsy1218 commented Feb 15, 2024

sismanis commented Feb 15, 2024

sismanis commented Feb 15, 2024

jsy1218 commented Feb 15, 2024

sismanis commented Feb 15, 2024

jsy1218 commented Feb 15, 2024 • edited Loading

sismanis commented Feb 15, 2024

sismanis commented Feb 15, 2024

jsy1218 commented Feb 16, 2024

sismanis commented Feb 16, 2024

jsy1218 commented Feb 18, 2024

jsy1218 commented Feb 23, 2024 • edited Loading

jsy1218 commented Feb 9, 2024 •

edited

Loading

jsy1218 commented Feb 15, 2024 •

edited

Loading

jsy1218 commented Feb 15, 2024 •

edited

Loading

jsy1218 commented Feb 23, 2024 •

edited

Loading