🐛 Improvement & Bug Fixes

[GabrielePicco]

Include changes from https://gist.github.com/yugure-orca/dfc8027455b2db8bdb2bac4cf255acad

Status	Type	⚠️ Core Change	Issue
Ready	Feature/Bug	Yes	-

Problem

See https://gist.github.com/yugure-orca/dfc8027455b2db8bdb2bac4cf255acad for detailed description

Discussion

Solana.Unity.Dex/IDex.cs

Swap

Isn't amountSpecifiedTokenType almost the same role as inputTokenMintAddress?

The typescript-SDK's amountSpecifiedIsInput is used to switch between ExactIn and ExactOut modes.

In ExactOut, you can specify that you want to swap to get 500 USDC.

👉This mode is especially important for payments, and should be used more often for in-game currency exchange.

Solution

• Removed amountSpecifiedTokenType and using amountSpecifiedIsInput to switch between ExactIn and ExactOut modes.
• Added a test case using ExactOut mode

OpenPositionWithLiquidity

I seem to recall an error when including an instruction to initialize a TickArray and an instruction to use a TickArray in the same transaction.

I think this is an Anchor issue, but if it happens in testing, the current workaround is to separate the transactions and execute them serially.

Comments

Doesn't seems to be the case, the two test cases that open and increase liquidity in one transaction are passing.

OpenPositionWithLiquidity & IncreaseLiquidity

If the methods only receive tokenMaxA and tokenMaxB, the concept of slippage will no longer exist.

It may be necessary to either receive quote or liquidity as well.

Solution

• Added slippage parameter and changed input parameter to tokenAmountA and TokenAmountB to compute max values from the given slippage
• Added OpenPositionWithLiquidityWithQuote and IncreaseLiquidityWithQuote that accepts quotes obeject as input
• Added tests cases

UpdateFeesAndRewards & UpdateAnd...

IncreaseLiquidity also use tickArrayLower and tickArrayUpper, but it do not accept them.

I think it is possible to get from the position.

Solution

• Removed from the input parameters and retrieved from the position.

CollectRewards

rewardMintAddress and rewardVaultAddress should be known from the pool data.

Compared to other methods, this argument seems to show low-level processing.

Solution

• Simplified and retrieved rewardMintAddress and rewardVaultAddress from the pool data

GetSwapQuote

Comment summary is the same as GetSwapQuoteFromWhirlpool?

Is it a method to find or route a Whirlpool?

Solution

• Updated the description. The GetSwapQuote allows to directly get a quote given the input/ouput mint

FindWhirlpoolAddress

There is a restriction on the order of tokenMintA and tokenMintB: SOL/USDC but not USDC/SOL.

We can either ask the user to enter tokenMintA and tokenMintB in this order, or we can find the pair without distinguishing between A and B.
Maybe the later is beginner-friendly.

Comment

The TryFindWhirlpool internally is already trying to find a pool for the reverse token mints, in case of failure with the given tokens order.

GetTokenBySymbol

Symbol is fine, but I think ByMint is safer, since it can be duplicated or changed (e.g. BTC to soBTC).

Solution

• Added a GetTokenByMint

missing? : FindPositions

I think that devs need a way to get a list of positions held by users.

https://github.com/everlastingsong/tour-de-whirlpool/blob/main/src_localization/EN/041_get_positions.ts

Solution

• Implemented and added test cases

General

commitment

I think that it is OK to take a default value of commitment from RPCClient.

If the default value is Finalized, many will specify Confirmed or Processed on each call.

Because Finalized takes approx 10-15 sec.

Solution

• All the methods now use the default commitment from the RPCClient, if no commitment is specified, so that it can be defined globally.

tickSpacing

https://github.com/garbles-labs/Solana.Unity-Core/blob/master/src/Solana.Unity.Dex/Ticks/TickSpacing.cs

It's hard to name, but a stable pool like USDC/USDT has tickSpacing 1.
I think the most general-purpose and usable tickSpacing is 64 (0.3% fee).
I think tickSpacing 128 (1% fee) is for more volatile tokens.

Comments

Happy to rename, but shouldn't we stay consistent with naming defined here: https://github.com/orca-so/whirlpools/blob/main/sdk/tests/utils/index.ts ?

Bugfixes

PR#66: orca-so/whirlpools#66

https://github.com/garbles-labs/Solana.Unity-Core/blob/master/src/Solana.Unity.Dex/Orca/Swap/SwapUtils.cs#L74

Solution

• Implemented the fix in the PR and added test cases

DecimalUtil

👉In my experience, Solana newbies get confused with Decimals. I think it would be good to have a utility for u64/Decimal conversion.

https://github.com/orca-so/orca-sdks/blob/main/packages/common-sdk/src/math/decimal-util.ts

Solution

• Implemented and added test cases

ATA for Wrapped SOL

I think it would be better to specify somewhere that the ATA for Wrapped SOL can be closed.

Solution

• Added a parameter unwrapSol to specify if the user want to close the the ATA for Wrapped SOL. True by default

Orca/Orca/OrcaDex.cs

L67

https://github.com/garbles-labs/Solana.Unity-Core/blob/master/src/Solana.Unity.Dex/Orca/Orca/OrcaDex.cs#L67

amountSpecifiedTokenType, tokenAuthority are not used

Solution

• Removed

L114

https://github.com/garbles-labs/Solana.Unity-Core/blob/master/src/Solana.Unity.Dex/Orca/Orca/OrcaDex.cs#L114

&& swapQuote.BtoA ?

Solution

• Fixed with && !swapQuote.AtoB

L334

https://github.com/garbles-labs/Solana.Unity-Core/blob/master/src/Solana.Unity.Dex/Orca/Orca/OrcaDex.cs#L334

Cannot close if fees or rewards remain

https://github.com/orca-so/whirlpools/blob/main/sdk/src/impl/whirlpool-impl.ts#L448

Solution

• Added collect fees and reward, if available, before closign

L353

https://github.com/garbles-labs/Solana.Unity-Core/blob/master/src/Solana.Unity.Dex/Orca/Orca/OrcaDex.cs#L353

If only max is specified, there is no room for slippage to be set.

If there is even a slight price movement, it will fail.

Solution

• Solved with above solution

L647

https://github.com/garbles-labs/Solana.Unity-Core/blob/master/src/Solana.Unity.Dex/Orca/Orca/OrcaDex.cs#L647

Can't find a pool with ts = 1?

Solution

• Added 1 to the search space

General

I wonder if ATAs need to be created for increaseLiquidity, decreaseLiquidity, collectFees and collectReward as well.

Comments

• All tests are passing, but I will investigate more

[coveralls]

Pull Request Test Coverage Report for Build 4224359399

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.004%) to 75.329%

---

Totals
Change from base Build 4084879553:	-0.004%
Covered Lines:	4849
Relevant Lines:	6150

---

💛 - Coveralls

[yugure-orca]

@GabrielePicco
Thanks for the feedback & updates!

FindWhirlpoolAddress

• It is OK to try A/B and B/A may, but the order can be pre-determined by the dictionary order of Mint addresses.
  https://orca-so.github.io/whirlpools/classes/PoolUtil.html#orderMints
• How about including tokenA and tokenB in the return value? The user will want to know whether it was A/B or B/A.

tickSpacing

https://github.com/orca-so/whirlpools/blob/main/sdk/tests/utils/index.ts
Sorry, I think this definition is old. Also it is the test code, SDK doesn't contain these defintion.
If we have to name one, 64 should be STANDARD.

WSOL/ATA

I have not been able to check the entire test case, but I think that ATAs were created with the pool when it was created and is always present.
Also, I am wondering whether or not there are test cases using pool where tokenA or tokenB is SOL.

If the following flow can be processed without any problem, it is OK!
Surprisingly, some users close ATAs for deposited tokens.
Perhaps they close empty ATAs to collect rent.

[case1] swap test 1
1.prepare: close ATA for WSOL
2.prepare: close ATA for mSOL
3.TX: swap SOL for mSOL using SOL/mSOL(1)

[case2] swap test 2
1.prepare: close ATA for WSOL
2.TX: swap mSOL for SOL using SOL/mSOL(1)

[case3] open / increase / decrease / collect / close
1.TX: open position in SOL/mSOL(1)
2.prepare: close ATA for WSOL
3.TX: increase liquidity
4.prepare: close ATA for WSOL
5.prepare: close ATA for mSOL
6.TX: decrease liquidity
7.prepare: close ATA for WSOL
8.prepare: close ATA for mSOL
9.TX: collect fees
10.prepare: close ATA for ORCA
11.prepare: close ATA for MNDE
12.TX: collect reward (0:ORCA)
13.TX: collect reward (1:MNDE)
14.TX: close position

[GabrielePicco]

Thanks @yugure-orca , those cases indeed identified some issues that I corrected.

---

FindWhirlpoolAddress

Added more information in the returned pool. The method now return a pool object with:

Address, TokenMintA, TokenMintB, Liquidity, Fee, TickSpacing

Renamed all spacing to literals to avoid any assumption

public struct TickSpacing
{
public const ushort One = 1;
public const ushort Eight = 8;
public const ushort ThirtyTwo = 32;
public const ushort SixtyFour = 64;
public const ushort HundredTwentyEight = 128;
}

Added several tests to check the described cases and fixed issues

• Add unit tests to check the above cases
• Fixed the associated issues

[yugure-orca]

I have made minor comments where relevant to ATA.
Since there are many changes in this PR, it would be acceptable to address them in subsequent PRs (IMO).
Especially since the ATA part is a behind-the-scenes job that does not affect the interface.

[GabrielePicco]

Thanks @yugure-orca for the comments, I included the latest changes & fixes. Merging in master and releasing version 1.0 🚀