-
Notifications
You must be signed in to change notification settings - Fork 191
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: handle shapeshift fees for jupiter and rate balance check #8267
Conversation
packages/swapper/src/swappers/JupiterSwapper/swapperApi/getTradeQuote.ts
Outdated
Show resolved
Hide resolved
packages/swapper/src/swappers/JupiterSwapper/swapperApi/getTradeRate.ts
Outdated
Show resolved
Hide resolved
packages/swapper/src/swappers/JupiterSwapper/swapperApi/getTradeRate.ts
Outdated
Show resolved
Hide resolved
packages/swapper/src/swappers/JupiterSwapper/swapperApi/getTradeRate.ts
Outdated
Show resolved
Hide resolved
packages/swapper/src/swappers/JupiterSwapper/swapperApi/getTradeRate.ts
Outdated
Show resolved
Hide resolved
packages/swapper/src/swappers/JupiterSwapper/swapperApi/getTradeRate.ts
Outdated
Show resolved
Hide resolved
packages/swapper/src/swappers/JupiterSwapper/swapperApi/getTradeRate.ts
Outdated
Show resolved
Hide resolved
packages/swapper/src/swappers/JupiterSwapper/swapperApi/getTradeRate.ts
Outdated
Show resolved
Hide resolved
packages/swapper/src/swappers/JupiterSwapper/swapperApi/getTradeRate.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
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.
Conceptually looks sane for my smol Solana brain minus a few non-domain-specific related comments, will continue in DM for disambiguation on specific concepts as required - runtime pass TBD up next
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, while this looks good overall, a few q.s regarding whether or not protocol fees are correct or not, as well as failing balance detection for small SOL account, regardless of ATA presence, ending up in reverts.
- SOL -> USDC I already have an ATA for ❔
q @NeOMakinG SOL protocol fees are neither 0.00204 SOL nor 0.00408 SOL, is this correct?
- USDC -> SOL on the same account - protocol fees are 0.00204 SOL ✅
- Swap of SOL to USDC to account without activity (no ATA) ❔
Protocol fees vary greatly and never seem to be 0.00204 SOL nor 0.00408, noticed e.g MSOL and BONK as protocol fees, is this expected?
Also another q here: quotes are often gotten with intricate fees (requiresBalance: false
) the user doesn't have. We currently did not have to deal with this ramification since fees were always in sell asset or fee asset, not in another asset altogether.
Wondering if we should surface that in the UI, bringing in some info somewhere with some copy like "Those fees do not require explicit payment. Those are intricate transaction fees taken during the swap" of sorts ? Also, should we add USD value next to them?
Captured #8302 for brainstorm
Actual trade is looking good though confirming those are non-payable ✅
https://jam.dev/c/c1bab2e7-4076-4af1-896f-1826ac4d53dc
- Swap of USDC to new token (no ATA) ❔
SOL protocol fees are neither 0.00204 SOL nor 0.00408 SOL
https://jam.dev/c/07bb700f-35cc-4485-a15c-d79a8aed5291
Unrelated to this PR: The swap failed at "Slippage exceeded" the first time around.
We probably want to follow-up with decoding those specific errors at broadcast time and surface them as "Slippage tolerance exceeded" error vs. blob of raw JSON currently? Captured in #8303
- Paranoia test of SOL to account without SOL balance ✅
Nice:
- Swap of smol SOL value to token with no ATA with account with smol balance :forbidden:
Last row fees are 0.00204 SOL regardless of amount input 🚫
q: Also not directly related to this PR @NeOMakinG: Having two SOL rows feel weird without any explanation of what these are, vs. them being aggregated. Do we want to follow-up with the notion of tagging fees with some human-readable description/tooltip? This could possibly be done in #8302
Actual swap execution is sad however:
https://jam.dev/c/f3c1cb8b-a753-4fa9-8af1-7ba54696a133
Phantom consistently fails with "An Account involved in this transaction does not have enough SOL" and if clicking the hidden "Confirm Anyway" button, /api/v1/send
will 400
- Swap of smol SOL value to token with ATA with account with smol balance :forbidden:
q: protocol fees do not always involve SOL, is this correct?
Execution failing same like the previous case
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/getTradeRate.ts
Outdated
Show resolved
Hide resolved
packages/swapper/src/swappers/JupiterSwapper/swapperApi/getTradeRate.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.
Retested locally with big boi (>1$ SOL) account
- Token -> SOL with ATA for token ✅
0.00204 SOL ✅
E2E: https://jam.dev/c/cf43b824-a5cf-4e78-afb0-301866a322fa ✅
- SOL -> token with ATA for token ✅
0.00204 SOL ✅
E2E: https://jam.dev/c/4edbdb44-d7c1-478f-841f-c45625aab5b6
- SOL -> token with no ATA for token ✅
0.00408 SOL ✅
E2E: https://jam.dev/c/916b129a-59e0-49f1-b17b-3c473199ddf9
And with smol boy account (<$0.7 SOL):
- SOL -> token with ATA for token 🚫
0.00204 SOL :question_mark:
@NeOMakinG this doesn't look correct given the 3 account creations:
Looks like this should be 0.00612 SOL?
https://jam.dev/c/b3e6b418-e2dc-4efe-9039-1d88993f08f9
- SOL -> token without ATA for token ✅
Balance checks are happy, and so is 0.00204 SOL ✅ 🎉
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.
Killer PR ser. Third time's the charm 🎉
Retested locally with big boi (>1$ SOL) account
- Token -> SOL with ATA for token ✅
0.00204 SOL ✅
E2E: https://jam.dev/c/865dd6d7-d052-4a28-9487-5bb69d002635 ✅
- SOL -> token with ATA for token ✅
0.00204 SOL ✅
E2E: https://jam.dev/c/7bf04611-c10c-41a0-935b-ef0e3e1bd7eb ✅
- SOL -> token with no ATA for token ✅
Insufficient balance
0.00408 SOL ✅
E2E: https://jam.dev/c/fcd4a25b-176a-4b58-a3d0-a17053e8936e ✅
And with smol boy account (<$0.7 SOL):
- SOL -> token without ATA for token ✅
Insufficient balance is detected correctly ✅
Fees looking sane (3 account creations)
- SOL -> token with ATA for token ✅
Insufficient balance is detected correctly ✅
Fees looking sane (3 account creations)
Removed high risk as the high risk bytes have been removed during the review and scoped to jupiter |
Description
ShapeShift referral fees require to create the referral account if it doesnt exists
Also, the ATA (Associated Token Account) creations fees were not taken in account in the protocol fees, and the balance check was not working properly on native assets
ATA: Your pubkey can have accounts linked on chain, if you want to own any solana token, you need an associated token account to hold them, users pay to create those account and can get back this rent price when they close the account if it's empty
Issue (if applicable)
closes #8247
Risk
Medium
Testing
If you want to verify referral account creation, create an account on https://referral.jup.ag/dashboard
And replace
SHAPESHIFT_JUPITER_REFERRAL_KEY
by yourReferral key
displayed in the dashboardYou have to monkeypatch
calculateFees
to return a BPS regardless of the fox discount or test with very big amounts, to trigger the referral account creationUsing a Solana wallet with big sol balance (let's say over 0.01):
-- If your wallet already have an ATA associated for Shitcoin, try with another token you never traded before
-- Always verifies the protocol fees are displaying 0.00204 SOL or 0.00408 SOL, or even more depending on the fact that there are some accounts created for multiple under the hood routes if we need to create the referral account (Jupiter instructions might create some in-between account that you need balance for, but they will be closed after the end of the TX exectution so you get back the amount)
Using a Solana wallet with very low sol balance (let's say under 0.003):
-- If your wallet already have an ATA associated for Shitcoin, try with another token you never traded before
-- Always verifies the protocol fees are displaying 0.00204 SOL or 0.00408 SOL, or even more depending on the fact that there are some accounts created for multiple under the hood routes if we need to create the referral account (Jupiter instructions might create some in-between account that you need balance for, but they will be closed after the end of the TX exectution so you get back the amount)
Engineering
n/a
Operations
n/a
Screenshots (if applicable)
Not enough balance
Not enough balance with a token I already have (referral account should be created)
Enough balance with a token I already own (referral account should be created)
https://explorer.solana.com/tx/5Lw6Xb1UXXDQhFRV6HtzxQ3TSw2LRe9e7ttWFNUM9N9aRWZY3dBg7qvEMt9iNYMKsWob2eGF5VfkyxpVBxtbLataEnough balance with a token I don't own (referral account should be created)
https://explorer.solana.com/tx/1XH9heWyCfbP3XmerY5e1p6Xr1p6yUCpTJvGAKX1Hf1aYc1hHKVNDKJh3WLeNZKUgfZFx1c3jXppFu6KzK4Y5c1Enough balance with a token I don't own (external address)
https://explorer.solana.com/tx/4AM1WSUgdSoJfHyXJkAWxD2Ro4JQQJYQABYY41jkLiQjbftybh6VKWPq3UaQoUcPVg6ceyuYCBypWc4qJ84UT1hX