-
Notifications
You must be signed in to change notification settings - Fork 189
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
feat: implement jupiter as a swapper #8120
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally:
- Unable to get rates without wallet connected (see inline comments for the reason why) 🚫
Wallet connected - no custom receive addy
- Able to get SOL sends rates with wallet connected ✅
- Able to get and execute SOL sell quote with wallet connected 🚫
https://jam.dev/c/bb637228-1a2d-4b72-af70-0d8644693849
Tx simulation is failing on Solana popup
And actual Tx is failing too
- Able to get SOL buy rates with wallet connected ✅
- Able to execute SOL buy quotes with wallet connected 🚫
Same failure as above
https://jam.dev/c/0bed7f93-1d0e-4ab6-b8e3-977ee8c81457
Custom receive buy addy
- Able to get SOL sends rates with wallet connected ✅
- Able to get and execute SOL sell quote with wallet connected 🚫
Managed to get unchained to return a Txid once (though never seemed to hit the mempool), and got the same error as above afterwards
https://jam.dev/c/b69e29f8-ea76-4635-ba8f-7049c2040ba7
- Should be unable to get SOL buy rates with wallet connected ✅ (cross-account execution not available for SOL buys)
- Able to get and execute SOL buy quote with wallet connected 🚫
https://jam.dev/c/1c567ab2-28ff-4cb4-b00f-dbe5a0478b21
Misc
- Able to get rates for Wrapped SOL ✅
@gomesalexandre we should be good for another review/runtime test 🙏 |
packages/swapper/src/swappers/JupiterSwapper/utils/constants.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing notes
Looks like some flakienss that needs to be addressed before I go deep on testing.
Unable to get SOL -> Wrapped SOL rate ❌
Unable to execute SOL -> USDC trade ❌
https://jam.dev/c/0ab18444-b0f6-4702-b481-2691a88e3a5f
Tried 4x more times, saw 3 failures as above and eventually got a trade to go through
Unable to execute USDC -> Wrapped SOL trade ❌
4 attempts:
https://jam.dev/c/2aa2cd51-dddf-4a86-b1d7-87ecb66e20f4
Successful USDC -> SOL trade ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can confirm the flakiness spotted by @woodenfurniture here: https://gist.github.com/gomesalexandre/bbf9f37e179b1ecc31e7505171ed08bb
Main takeaways of this testing:
- SOL <-> Wrapped SOL rates are borked (and their execution would probably be too if we could get to this stage with current diff)
- Execution in general is flaky - either with reverts during simulation as can be seen in Phantom, and/or with no reverts, but not hitting the mempool seemingly (FYI, getting Txids from unchained's
/api/v1/send
endpoint is a red herring, it doesn't mean the Tx actually hit the mempool, it may still not. It only means got a deterministic Txid, and the Tx itself may or may not have hit the mempool) - Our ws seems flaky, though not related to this PR per se
q1. WRT execution @NeOMakinG: are we sure that the current way of keeping the rate from rate and only adding additional props to it yields consistent successes?
To me, it seems like getting a final quote the usual way with the right compute units is the way, and getting said additional data at quote time while keeping the original rate data is a recipe for flakiness, but this may or may not be the correct way for Solana, not sure.
q2.: how much of a lift would it be to add auto-slippige here for Jupiter as a first swapper to ensure better reliability (and rates while at that), and do we want to do that in this PR to mitigate failures here?
q1 @gomes : Also tried to wait around 40 minutes after fetching the quote, it did fail, so I decided to fetch the prices again in the getTradeQuote handler in [5780800] (5780800) Can you please guys test again and tell me if you see any more flaky behaviors? q2: From the Jupiter point of view, the list is not so big: Update But please, we should try this in a follow up if we really want to, not on this particular PR which starts to be big |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answered all the comments
packages/swapper/src/swappers/JupiterSwapper/swapperApi/getTradeQuote.ts
Outdated
Show resolved
Hide resolved
packages/swapper/src/swappers/JupiterSwapper/swapperApi/getTradeQuote.ts
Outdated
Show resolved
Hide resolved
packages/swapper/src/swappers/JupiterSwapper/swapperApi/getTradeQuote.ts
Outdated
Show resolved
Hide resolved
packages/swapper/src/swappers/JupiterSwapper/swapperApi/getTradeQuote.ts
Show resolved
Hide resolved
packages/swapper/src/swappers/JupiterSwapper/swapperApi/getTradeQuote.ts
Outdated
Show resolved
Hide resolved
packages/swapper/src/swappers/JupiterSwapper/models/AccountMeta.ts
Outdated
Show resolved
Hide resolved
packages/swapper/src/swappers/JupiterSwapper/utils/constants.ts
Outdated
Show resolved
Hide resolved
packages/swapper/src/swappers/JupiterSwapper/swapperApi/getTradeQuote.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stamping as I ended up with the simulation failed on Phantom / Tx not broadcasted to the mempool only once, and couldn't repro after (which is why there is none in this Jam) despite many attempts). E2E testing below:
https://jam.dev/c/4bae293f-4fb2-47eb-85ab-1b53d231642e
Still noticing some Txs rarely failing for a diff. reason, though increasing slippeedge seems to solve it, so not too concerned about this as we will fast-follow with auto-slippage (#8193)
@NeOMakinG if you feel like it as a fast-follow, the ETA as " ms" where x < 100 is still looking very odd to me, looks like it got lost in translation in previous review (https://gist.github.com/gomesalexandre/bbf9f37e179b1ecc31e7505171ed08bb) but would be nice to tackle as a one-liner fast-follow, or in this PR if you feel like it, to make it no ETA similar to most EVM swappers (see 0x and Portals in the screenshot below missing it altogether, since swap is done within a block i.e effectively instantly, vs. Jup showing some ETA in the realm of ms):
Removed the time for jupiter in 561cd08 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed swaps are looking much more stable now.
https://jam.dev/c/0f249b0c-7c8d-47b1-9a67-060da1fb2e16
https://jam.dev/c/6e14d3dd-137f-4beb-8cec-b7994d786d5e
Lets get this in and enjoy the follow-ups when they come 🎉
Description
Implementing the Jupiter swapper:
Issue (if applicable)
closes #8017
Risk
High because swapper but behind a feature flag
Testing
Engineering
n/a
Operations
n/a
Screenshots (if applicable)
SOL to SPL
SPL to SOL
https://jam.dev/c/ce54f3bb-43f1-4e1f-9f44-c8b44a19fb73SPL to SPL
https://explorer.solana.com/tx/3jfq6B3z44Y7ffJxfVm2wRAKFJMba8fANUitHSguzmiKgb8NYfyRfWRHyqvokxHbRe8Qi5mUyuESoAuUEZzwTau5
https://jam.dev/c/16c8e5f0-d5e2-4530-865b-b324a6980e25
TX history
Txs links
https://explorer.solana.com/tx/23xAVVo18sgD2ykyqH5Kx9C37j6bLNjbFXi2rRW4ge5YArm1ywsTFGXKzSRmAQV6Dx13GnXZP5W7f1UNjmrAqDau
https://explorer.solana.com/tx/5k2r33Dd2oPKf5NESmQZz2G2fQ23wSy7xfL82PhCKwZuT5kojmynuHrVDy3uzhsf2UFq13wKjcvJMr61cdsGDGBL
https://explorer.solana.com/tx/2ucg29ZHH58uVZ9MCbE3VCCfwgHVdcozcxG4NfELDmTQTew4T6HQKh6E8eBx7Xr12K2xjZEEHpHDfdi1fQxg7Tyv
https://explorer.solana.com/tx/4XgfBYeqrn5pq2vTGmYrktYmgii1EiUXR7vZNzmULNNQgePMQVMKZsVWPbChURtUrYN9w8cD2aFteVtP1fCet8LV