Skip to content
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

Merged
merged 10 commits into from
Dec 6, 2024

Conversation

NeOMakinG
Copy link
Collaborator

@NeOMakinG NeOMakinG commented Dec 4, 2024

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

High Risk PRs Require 2 approvals

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

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 your Referral key displayed in the dashboard

You have to monkeypatch calculateFees to return a BPS regardless of the fox discount or test with very big amounts, to trigger the referral account creation

Using a Solana wallet with big sol balance (let's say over 0.01):

  • Try to swap SOL => Shitcoin, Shitcoin => SOL
    -- 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):

  • Try to swap SOL => Shitcoin, Shitcoin => SOL
    -- 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

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)

Screenshots (if applicable)

Not enough balance

image

Not enough balance with a token I already have (referral account should be created)

image

Enough balance with a token I already own (referral account should be created)

image image https://explorer.solana.com/tx/5Lw6Xb1UXXDQhFRV6HtzxQ3TSw2LRe9e7ttWFNUM9N9aRWZY3dBg7qvEMt9iNYMKsWob2eGF5VfkyxpVBxtbLata

Enough balance with a token I don't own (referral account should be created)

image https://explorer.solana.com/tx/1XH9heWyCfbP3XmerY5e1p6Xr1p6yUCpTJvGAKX1Hf1aYc1hHKVNDKJh3WLeNZKUgfZFx1c3jXppFu6KzK4Y5c1

Enough balance with a token I don't own (external address)

image image https://explorer.solana.com/tx/4AM1WSUgdSoJfHyXJkAWxD2Ro4JQQJYQABYY41jkLiQjbftybh6VKWPq3UaQoUcPVg6ceyuYCBypWc4qJ84UT1hX

@NeOMakinG NeOMakinG marked this pull request as ready for review December 5, 2024 08:50
@NeOMakinG NeOMakinG requested a review from a team as a code owner December 5, 2024 08:50
@NeOMakinG NeOMakinG marked this pull request as draft December 5, 2024 08:50
@NeOMakinG NeOMakinG changed the title feat: handle shapeshift fees for jupiter feat: handle shapeshift fees for jupiter and rate balance check Dec 5, 2024
@NeOMakinG NeOMakinG marked this pull request as ready for review December 5, 2024 12:48
Copy link
Contributor

@gomesalexandre gomesalexandre left a 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

Copy link
Contributor

@gomesalexandre gomesalexandre left a 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 ❔
image

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 ✅
image
  • 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) ❔
image

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 ✅
image

Nice:

image
  • Swap of smol SOL value to token with no ATA with account with smol balance :forbidden:
image

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
image

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?

image

Execution failing same like the previous case

https://jam.dev/c/771460cb-18f9-427f-9b93-170c48b2e8a1

@NeOMakinG
Copy link
Collaborator Author

I dont have an ATA + we don't have a referral account

image

I have an ATA + we don't have a referral account

image

Both have ATA and referral account

image

Having 2 SOL rows

We have 2 SOL rows because one is not as required balances and is a part of the JUP protocol, the other is our own logic as required balances

Having other tokens as protocol fee

Jupiter under the hood can use underlying assets as fees (for protocol or affiliate), each route can have it's one denominated token as fee, I display all of them inside this part

Copy link
Contributor

@gomesalexandre gomesalexandre left a 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 ✅

image

0.00204 SOL ✅

E2E: https://jam.dev/c/cf43b824-a5cf-4e78-afb0-301866a322fa

  • SOL -> token with ATA for token ✅

image

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 🚫

image

0.00204 SOL :question_mark:

@NeOMakinG this doesn't look correct given the 3 account creations:

image

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 ✅ 🎉

image

Copy link
Contributor

@gomesalexandre gomesalexandre left a 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 ✅

image

0.00204 SOL ✅

E2E: https://jam.dev/c/865dd6d7-d052-4a28-9487-5bb69d002635

  • SOL -> token with ATA for token ✅

image

0.00204 SOL ✅

E2E: https://jam.dev/c/7bf04611-c10c-41a0-935b-ef0e3e1bd7eb

  • SOL -> token with no ATA for token ✅

image

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 ✅

image

Fees looking sane (3 account creations)

image

  • SOL -> token with ATA for token ✅

Insufficient balance is detected correctly ✅

image

Fees looking sane (3 account creations)

image

@NeOMakinG
Copy link
Collaborator Author

Removed high risk as the high risk bytes have been removed during the review and scoped to jupiter

@NeOMakinG NeOMakinG enabled auto-merge (squash) December 6, 2024 16:37
@NeOMakinG NeOMakinG merged commit 4951e9c into develop Dec 6, 2024
3 checks passed
@NeOMakinG NeOMakinG deleted the affiliate-jupiter branch December 6, 2024 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Research and implement token account creation for Solana affiliate fees
2 participants