TokenExtensions (Contract+SDK) #134
Conversation
| pub token_program_a: Interface<'info, TokenInterface>, | ||
| #[account(address = token_mint_b.to_account_info().owner.clone())] | ||
| pub token_program_b: Interface<'info, TokenInterface>, | ||
| pub memo_program: Program<'info, Memo>, |
There was a problem hiding this comment.
Do we really need to always include the memo program (for memo-required transfers I presume)? Wouldn't it be easier to add the memo from sdk side in a separate instruction? IIRC it doesn't matter where the memo instruction is as long as it happened before the transfer that requires the memo
There was a problem hiding this comment.
Thanks for checking. I've checked the specification again.
Yep, I believe it is required.
As you know, we need to consider the case whirlpool is used via CPI by other programs such as Jupiter.
And, memo instruction RIGHT before the transfer is required.
https://spl.solana.com/token-2022/extensions#required-memo-on-transfer
By enabling required memo transfers on your token account, the program enforces that all incoming transfers must have an accompanying memo instruction right before the transfer instruction.
In the implementation, the only 1 previous sibling instruction is checked.
https://github.com/solana-labs/solana-program-library/blob/master/token/program-2022/src/extension/memo_transfer/mod.rs#L47
get_processed_sibling_instruction(0):
https://docs.rs/solana-program/1.18.8/src/solana_program/instruction.rs.html#680
So if user enabled MemoTransfer for both owner accounts, decrease_liquidity_v2 will execute...
#1 Whirlpool (decrease_liquidity_v2)
+-- 1.1 Memo
+-- 1.2 TokenProgram (transferChecked valut to user)
+-- 1.3 Memo
+-- 1.4 TokenProgram (transferChecked vault to user)
- Sibling(0) of 1.2 is 1.1
- Sibling(0) of 1.4 is 1.3
- If we omit "1.3 Memo", "1.4 TokenProgram" will not recognize "1.1 Memo" and say that "1.2 TokenProgram" is not memo.
TokenExtension is not CU & tx size friendly...🤦
There was a problem hiding this comment.
If we don't use the memo program for the transfer fee log, then would it be possible to include the memo program in the remaining_accounts and only deserialize the Memo account if any of the tokens require it?
|
|
||
| pub fn handler<'a, 'b, 'c, 'info>( | ||
| ctx: Context<'a, 'b, 'c, 'info, CollectProtocolFeesV2<'info>>, | ||
| remaining_accounts_info: RemainingAccountsInfo |
There was a problem hiding this comment.
Looked a little bit but couldn't figure out where the remaining_accounts_info comes from. Is this something we added or from Anchor? What is the format of these since it looks like we're splitting them based on which mint they belong to (which makes sense)
There was a problem hiding this comment.
remaining_accounts_info is passed as a part of data of the instruction.
I described in detail in the following comment. 🙏
#134 (comment)
| } | ||
|
|
||
| // reject if mint has freeze_authority | ||
| if token_mint.freeze_authority.is_some() && !is_token_badge_initialized { |
There was a problem hiding this comment.
Should we also block token_mints with freeze_authority if they are token program (non-2022)? We can use the same token badge to allow certain mints?
There was a problem hiding this comment.
Unfortunately, we cannot block freeze authority on TokenProgram/TokenProgram pairs unless initialize_pool (v1) is deprecated and removed.
To maintain the state that what can be done in v1 can also be done in v2, tokens that use TokenProgram are allowed now.
There was a problem hiding this comment.
I'd say it might be worth adding it here (even though users can currently still circumvent using the v1 instruction)
| extension::ExtensionType::MetadataPointer => {} | ||
| // partially supported | ||
| extension::ExtensionType::ConfidentialTransferMint => { | ||
| // Supported, but non-confidential transfer only |
There was a problem hiding this comment.
Think we need a check here if non-confidential transfers are allowed on the mint. If not users can create a pool with tokens that just don't work (since we call TransferChecked that fails if non-confidential transfers is not allowed)
There was a problem hiding this comment.
hmm, in my understanding, ConfidentialTransfer extension at Mint account doesn't have option to force everyone use confidential transfer.
At (Token)Account account, allow_non_confidential_credits exists, and user can disable non-confidential transfer into the account.
Vault is of course not set to false.
If user disable non-confidential transfer on their own account, it is their problem.
WDYT ?
There was a problem hiding this comment.
Got it. Thought allow_non_confidential_credits was an option on the mint but that is not the case. 👍🏼
| Ok(ctx | ||
| .accounts | ||
| .token_badge | ||
| .initialize( |
There was a problem hiding this comment.
So just the existence of a token badge account allows things like permanent delegate and freeze authority?
There was a problem hiding this comment.
Yes.
My first thought was to have a flag to allow for each extension. (such as allowPermanentDelegate, allowTransferHook ...)
But at some point I realized that in the end it was only a 0 or 1, whether or not to allow the pool to be created using that token per WhirlpoolsConfig. Also, dangerous extension is only initializable at Mint initialization. So for example, adding PermanentDelegate extension after the initialization is impossible.
I reserved 128 bytes of expansion room just in case.
| pub config_extension_authority: Signer<'info>, | ||
|
|
||
| /// CHECK: safe, the account that will be new authority can be arbitrary | ||
| pub new_config_extension_authority: UncheckedAccount<'info>, |
There was a problem hiding this comment.
Did we really need a v2 for this or did we just create v2 instructions for everything so that at some point we can phase out all the v1 instructions more easily?
There was a problem hiding this comment.
TokenBadge and WhirlpoolsConfigExtension are not "_v2" instruction.
They are included in /v2/ because they are implemented for TokenExtension.
If you feel uncomfortable with the location of the source code, we can change it.
In addition, I would like to add that I have separated the dir/file to make it as clear as possible that the v1 code has not been changed. (Not destroying v1 is an important point for this change)
There was a problem hiding this comment.
Yep think the current setup makes sense to leave v1 as is
| pub new_token_badge_authority: UncheckedAccount<'info>, | ||
| } | ||
|
|
||
| /// Set the fee authority. Only the config extension authority has permission to invoke this instruction. |
There was a problem hiding this comment.
This comment seems to be talking about something else? What does the TokenBadgeAuthority allow you to do? Just initialize token badge accounts?
There was a problem hiding this comment.
For example, feeAuthority can set newFeeAuthority by signing transaction by themself. (set_fee_authority instruction can be executed by the signature of the current feeAuthority).
On the other hand, tokenBadgeAuthority cannot set newTokenBadgeAuthority by themself.
configExtensionAuthority need to do it.
This is designed to allow for the addition of new fields to WhirlpoolsConfigExtension easier.
WhirlpoolsConfigExtension has 512 bytes of reserved space that can be used for future additions.
Since the reserved area is initialized to 0, adding functionality without breaking the already created WhirlpoolsConfigExtension would involve adding a new setX instruction and having configExtensionAuthority execute it.
This is because newly adding authority is uninitialized and cannot set itself.
Rather than adding two sets of instructions, one for initialization and one for updating itself, one instruction can be added by using only configExtensionAuthority.
There was a problem hiding this comment.
But this is the instruction for setting the badge authority why is the comment talking about a fee authority?
There was a problem hiding this comment.
aaaaah, you are right!
This comment is wrong. it syould be Set the token badge authority..
I'll update. 🫡
| pub fn parse_remaining_accounts<'info>( | ||
| remaining_accounts: &[AccountInfo<'info>], | ||
| remaining_accounts_info: &RemainingAccountsInfo, | ||
| valid_accounts_type_list: &[AccountsType], |
There was a problem hiding this comment.
I understand what this function is supposed to do: take a list of remaining_accounts and split them up based on what mint they belong to. But I don't fully understand yet how this function is doing that. Where does remaining_account_info come from? Is this something that needs to be specified in the instruction data?
There was a problem hiding this comment.
remaining_account_info is not an Anchor provided function.
It is a simple data passed as data of an instruction.
At client-side, we need to build remainingAccountsInfo data by the following Util class.
https://github.com/orca-so/whirlpools/pull/134/files#diff-1ab45321ec7b9a26165920668ef546656e1fe0d49a6de83c9f0e8aa9352a4cdc
This class is used as follows.
https://github.com/orca-so/whirlpools/pull/134/files#diff-e87787e30f5ee571d972a7cd9b32aa56091ead71895bd8e13fdf0d61c0405f2f
There is flexibility to add a variety of things to remaining_accounts.
However, unless the role of each account is made explicit, we will have to carefully check and categorize account owners, etc. If swap_v2 will be extended to accept additional TickArray accounts, or if other accounts are needed, this data could be used for would make it possible to use remaining_accounts in a structured manner.
There was a problem hiding this comment.
regarding passing in the role of each account; if the remaining_accounts are guaranteed to be in a consistent order w.r.t to the caller:
can we force callers to just input them in a specific order and thus remove the constraint of having to provide the accounts_type field?
Each instruction can preserve an ordering to the accounts, and since these are user provided accounts, they will need to be verified manually anyways such as checking the transfer hook account owner
This would be roughly equivalent to how transactions are constructed now (where accounts need to provided in a specific order anyways)
| if let Some(epoch_transfer_fee) = get_epoch_transfer_fee(token_mint)? { | ||
| // log applied transfer fee | ||
| // - Not must, but important for ease of investigation and replay when problems occur | ||
| // - Use Memo because logs risk being truncated |
There was a problem hiding this comment.
This requires extra compute units and logs to the transaction log anyway (which is the same as just doing msg! ourselves? In Solana v1.18 transactions that require less CUs get more prio so there is now an incentive to keep the CUs as low as reasonably possible
There was a problem hiding this comment.
Thanks for the comment!
Logs may be truncated, but instructions that remain as innerInstruction are never truncated and we can "data" as log.
This is the advantage of using Memo.
I looked into this issue again and found that the following noop program is used in account-compression.
It appears to consume much less CU than Memo, but the down side is to make sure that the noop_program is received in the v2 instruction. (extra 32 bytes without ALT)
https://solana.stackexchange.com/questions/10090/cost-of-logging-vs-storing-data-in-solana-programs/10100#10100
https://crates.io/crates/spl-noop
no-op program: https://solscan.io/account/noopb9bkMVfRPU8AsbpTUg8AQkHtKwMYZiFUjNRtMmV
WDYT about the reason to use Memo and noop program ?
There was a problem hiding this comment.
Since Memo already needs to be there for the memo-required extension we might as well just use that. I think the impact on CUs will not be that large whereas having to add the noop_program everytime adds to the tx size which is currently sometimes already an issue (harvest, withdraw with ata creation, etc.)
There was a problem hiding this comment.
Thanks for sharing your thoughts.
Agreed. Let's move forward with Memo program.👍
| extension::ExtensionType::DefaultAccountState => { | ||
| if !is_token_badge_initialized { return Ok(false); } | ||
|
|
||
| // reject if default state is not Initialized even if it has token badge |
There was a problem hiding this comment.
What about if the default account state is frozen? I think this will mess with the token vaults of the pool. i.e. the freeze authority will need to unfreeze the vaults before the pool becomes usable
There was a problem hiding this comment.
I agree to reject mint with "Frozen" default state even if it has TokenBadge.
If the current default state is not "Initialized" (not "Initialized" = "Frozen"), I believe the current implementation rejects it by the following code (L:269-273). Am I missing something ?
Also it is tested in the following file.
sdk/tests/integration/v2/initialize_pool_v2.test.ts
L:1076
L:1088
(Direct link doesn't work due to many diff ?)
note: GYEN has DefaultAccountState extension but its default state is "Initialized": https://solana.fm/address/Crn4x1Y2HUKko7ox2EZMT6N2t2ZyH7eKtwkBGVnhEq1g/extensions?cluster=mainnet-alpha
There was a problem hiding this comment.
Got it. The DefaultAccountState is just en enum. I thought it was more sophisticated
|
I think potentially we can support
and
https://solana.com/developers/guides/token-extensions/interest-bearing-tokens |
|
@wjthieme My understanding of InterestBearing may still be limited, but I am very cautious about this extension. I am concerned about whether the difference between AMM and CLAMM is consistent with InterestBearing. In simple CP-AMM, the number of tokens determines the price, so an increase in the number of tokens is automatically reflected in the price and no inconsistency occurs. In CLAMM, on the other hand, there is no relationship between the amount of tokens in the vault and the price. Even if tokens were sent directly to the vault, the price would not change. WDYT? It is possible that I am just overly concerned because I am a worrier. 😂 |
IIRC the interesting bearing token extension does not actually increase any Because of this I would say it would be relatively safe to add since token amounts don't actually change with this extension. Not 100% sure what would happen with accrued interest (since it is just a UI thing) if tokens are transferred |
|
@wjthieme However, I don't think we should be in a hurry to enable it, and we will have to add test cases along with the addition. I would like to proceed with this PR to an audit process, so I would like to avoid adding extensions now that were not part of the original plan (=c1 pitch). |
|
✋Subsequent updates will be made in a separate branch for auditing purposes. |
* add testcase: re-initialize TokenBadge * make TokenExtensionUtil public * use V2 instructions * deprecate PriceModule, WhirlpoolRouter * add isSupportedToken (client side) * add client test * client impl update
|
[audit feedback] For the following two reasons, I would NOT adopt checking the amount before and after TransferChecked. 1. TransferHook program can never manipulate the amount
Checked code2. TransferFee calculation is sufficiently robust to ensure that it is correctcalculate_transfer_fee_excluded_amountThis process is far simpler than the reverse process. calculate_transfer_fee_included_amountThere are several edge cases and this is a process that requires attention. However, the following code for the final result guarantees the correctness of the reverse operation. I had completely forgotten that I had this verification built in and was very relieved when I rediscovered it, lol. |
|
Audit have been completed (~ 768f8d7 commits have been audited). |
* Price-based slippage calculation for increase liquidity (#135) - Deprecated the old increaseLiquidityQuoteByInputToken quote function & added a new increaseLiquidityQuoteByInputTokenUsingPriceSlippage to calculate tokenMax based on price movement - Adding increaseLiquidityQuoteByLiquidity quote function to generate a quote by a set liquidity value * Bump common-sdk to 0.5.2 (#138) * Bump common-sdk to 0.5.2 * Lockfile * Version bump --------- Co-authored-by: Michael Hwang <[email protected]> Co-authored-by: Wilhelm Thieme <[email protected]> * Bump common-sdk to 0.5.3 (#139) * Bump common-sdk to 0.5.3 * Bump whirlpools-sdk to 0.12.4 * Remove some unnused/unnecessary lock files (#137) * use idempotent on whirlpool.swap (swapAsync) (#142) * use idempotent on whirlpool.swap (swapAsync) * add test case * bump version 0.12.5 * fix compile error --------- Co-authored-by: meep <[email protected]> Co-authored-by: tmoc <[email protected]> Co-authored-by: Michael Hwang <[email protected]> Co-authored-by: Wilhelm Thieme <[email protected]> Co-authored-by: Will <[email protected]>
| pub fn parse_remaining_accounts<'info>( | ||
| remaining_accounts: &[AccountInfo<'info>], | ||
| remaining_accounts_info: &RemainingAccountsInfo, | ||
| valid_accounts_type_list: &[AccountsType], |
There was a problem hiding this comment.
regarding passing in the role of each account; if the remaining_accounts are guaranteed to be in a consistent order w.r.t to the caller:
can we force callers to just input them in a specific order and thus remove the constraint of having to provide the accounts_type field?
Each instruction can preserve an ordering to the accounts, and since these are user provided accounts, they will need to be verified manually anyways such as checking the transfer hook account owner
This would be roughly equivalent to how transactions are constructed now (where accounts need to provided in a specific order anyways)
| pub token_program_a: Interface<'info, TokenInterface>, | ||
| #[account(address = token_mint_b.to_account_info().owner.clone())] | ||
| pub token_program_b: Interface<'info, TokenInterface>, | ||
| pub memo_program: Program<'info, Memo>, |
There was a problem hiding this comment.
If we don't use the memo program for the transfer fee log, then would it be possible to include the memo program in the remaining_accounts and only deserialize the Memo account if any of the tokens require it?
|
|
||
| // Store the fees owed to use as transfer amounts. | ||
| let fee_owed_a = position.fee_owed_a; | ||
| let fee_owed_b = position.fee_owed_b; |
There was a problem hiding this comment.
just a note: but I think with the introduction of TransferFees, we might want to be a bit more careful in how we display these FeeOwedA and FeeOwedB type of values, since this will be discounted by the transfer fee rate
There was a problem hiding this comment.
remaining_accounts_info
To reduce data size, I've updated its data type to Option<RemainingAccountInfo>.
Now it is just used to provide accounts required by TransferHook program.
To use TransferHook, TokenBadge is required, so very limited number of tokens can use this hook.
So many instruction doesn't use remaining_accounts.
So Option will reduce data size a bit for "no remaining accounts" case.
#147
data size of Option<RemainingAccountsInfo>:
- (no remaining accounts): 1 byte
- (some remaining accounts): 1 byte + 4 byte (vec length, u32) + (1 byte + 1 byte) x
I've checked data size for enum, it is u8.
Memo program
(no update on this PR)
Whether MemoTransfer is required is set by the TokenAccount account, not the Mint account.
Therefore, even if Mint has been fetched, the ATA for each user must be fetched to determine MemoTransfer is required at client-side. I currently think that it is nice to provide a Memo program that can be used by on-chain if needed.
We can use ALT to mitigate the size increase caused by the Memo program.
| use anchor_spl::token_interface::Mint; | ||
|
|
||
| #[derive(Accounts)] | ||
| pub struct DeleteTokenBadge<'info> { |
There was a problem hiding this comment.
Should be okay, but it might be nice to have a test that
is_token_badge_initialized = false
Initialize TokenBadge
is_token_badge_initialized = true
Delete TokenBadge
is_token_badge_initialized = false
Initialize TokenBadge
is_token_badge_initialized = true
To ensure that this behavior would work as expected (I think according to the code, it should, but would be good to test)
There was a problem hiding this comment.
Would the following test case help increase our confidence?
whirlpools/sdk/tests/integration/v2/token-badge/delete_token_badge.test.ts
Lines 423 to 452 in e99821a
There was a problem hiding this comment.
yup 🔥 ! I didn't see this since I hadn't reviewed the ts code yet
| pub new_config_extension_authority: UncheckedAccount<'info>, | ||
| } | ||
|
|
||
| /// Set the fee authority. Only the current config extension has permission to invoke this instruction. |
There was a problem hiding this comment.
nit: /s/fee authority/config extension authority
There was a problem hiding this comment.
good catch! 🙏
updated on #147
| Ok(ctx | ||
| .accounts | ||
| .whirlpools_config_extension | ||
| .update_config_extension_authority(ctx.accounts.new_config_extension_authority.key())) |
There was a problem hiding this comment.
should we add an additional instruction similar to the super-authority for the whirlpools config? we don't need to do it now, but theoretically, it is possible if at some point the config authority accidentally sets the wrong new_config_extension_authority to a key that nobody has the signer for and we lose control of the config extension
There was a problem hiding this comment.
I think we are talking about set_reward_authority_by_super_authority and set_reward_emissions_super_authority.
In my understanding, reward_authority for each reward is set on Whirlpool account.
I think super_authority is a mechanism for "delegation" and to bail out in case the "delegated" third party loses or misuses the authority.
So I don't think it is used to rescue the case where an important authority included in WhirlpoolsConfig is mistakenly set.
Now there is no instruction to rescue the case where the authority defined in WhirlpoolsConfig is mistakenly set.
Therefore, I think the same should be applied to the authority of WhirlpoolsConfigExtension.
(If such a situation should occur, I think we need to add a rescue instruction.)
| ); | ||
|
|
||
| // amount | ||
| let input_amount = if is_token_fee_in_one_a { swap_update_one.amount_a } else { swap_update_one.amount_b }; |
There was a problem hiding this comment.
nit: could combine these two to show they are both switching on is_token_fee_in_one_a
There was a problem hiding this comment.
updated on #147
sdk/src/impl/position-impl.ts
Outdated
| tokenMintB: whirlpool.tokenMintB, | ||
| tokenProgramA: tokenExtensionCtx.tokenMintWithProgramA.tokenProgram, | ||
| tokenProgramB: tokenExtensionCtx.tokenMintWithProgramB.tokenProgram, | ||
| tokenTransferHookAccountsA: await TokenExtensionUtil.getExtraAccountMetasForTransferHook( |
There was a problem hiding this comment.
nit: not sure how long the getExtraAccountMetasForTransferHook takes, but since it includes a getAccountInfo, it might be slightly more performant to parallelize the tokenTransferHookAccountsA/B?
There was a problem hiding this comment.
Good catch!
I've added TokenExtensionUtil.getExtraAccountMetasForTransferHookForPool to get metas for A/B in parallel.
| let posIndex = 0; | ||
| // build tasks | ||
| // For TokenProgram-TokenProgram pair pool, collectFees and 3 collectReward instructions can be packed into one transaction. | ||
| // But if pool has TokenExtension, especially TransferHook, we can no longer pack all instructions into one transaction. |
There was a problem hiding this comment.
out of curiosity, is this true even if all accounts were included in an ALT?
There was a problem hiding this comment.
The situation would improve if ALTs can be used.
It has been a long time since Versioned Transaction was released and I feel it is time for the UI (including supported wallet) and SDK to stop supporting legacy transactions.
Looking at an analysis of the wallet apps of Orca users, many users use Phantom.
| .filter((_, i) => { | ||
| return ( | ||
| (rewardsQuote.rewardOwed[i] ?? ZERO).gtn(0) || | ||
| // we need to collect reward even if all reward will be deducted as transfer fee |
There was a problem hiding this comment.
(don't have to change for this PR), but I think it might be good to add additional handling on the caller side to basically have a confirmation or force flag for reward and fee collections since there may be scenarios where it might benefit a user more to have a better understanding of how the fees may be calculated.
One specific example, such as collecting protocol fees for our fee conversions.
If we have a script that regularly checks to see if there are uncollected fee conversions and attempts to collect too frequently, we might end up with less than optimal collection
Let's say that the transfer fee fee rate is 100% (although this applies to lower percentages as well).
We may end up in a state where every time we attempt to collect fees, we collect fees less than the maximum_fee, and end up actually receiving 0. vs. If we can detect on the caller that we would receive 0 fees, then we could try to optimize when to collect (letting the fees accrue beyond the maximum_fee to some threshold)
There was a problem hiding this comment.
yeah, good point. frequent collections can be a disadvantage in those cases.
(no update on this PR)
| tokenExtensionCtx.currentEpoch | ||
| ); | ||
|
|
||
| rewardOwed[i] = transferFeeExcluded.amount; |
There was a problem hiding this comment.
What do you think about applying a similar pattern here to the rust TransferFeeIncludedAmount and TransferFeeExcludedAmount and forcing callers to handle the amounts in a more specific way rather than callers using the naming convention in transferFee to determine the relationship between the amount and the fee?
So for example,
export type CollectRewardsQuote = [
TransferFeeExcludedAmount | undefined,
TransferFeeExcludedAmount | undefined,
TransferFeeExcludedAmount | undefined,
]
There was a problem hiding this comment.
I've added TransferFeeIncludedAmount and TransferFeeExcludedAMount type at #147
It is used as return type of calculateTransferFeeIncludedAmount and calculateTransferFeeExcludedAmount.
I kept data type of quote objects.
- To minimize breaking change
- The fees consumed in a trade are stored in the estimatedFeeAmount field; the transferFee field is similar one, IMO.
| public static isV2IxRequiredPool( | ||
| tokenExtensionCtx: TokenExtensionContextForPool | ||
| ): boolean { | ||
| if (tokenExtensionCtx.tokenMintWithProgramA.tokenProgram.equals(TOKEN_2022_PROGRAM_ID)) return true; |
There was a problem hiding this comment.
nit:
return tokenExtensionCtx.tokenMintWithProgramA.tokenProgram.equals(TOKEN_2022_PROGRAM_ID)
|| tokenExtensionCtx.tokenMintWithProgramB.tokenProgram.equals(TOKEN_2022_PROGRAM_ID);
?
There was a problem hiding this comment.
updated at #147
* fix comment * combine two statements depending on same condition * refactor isV2IxRequiredPool * refactor: split TokenAmountWithFee into 2 explicit types * type change BN to TransferFeeIn(Ex)cludedAmount on quotes * Revert "type change BN to TransferFeeIn(Ex)cludedAmount on quotes" This reverts commit 41008b1. * fix Router test case (compatibility) * make remaining_accounts_info Option * add getExtraAccountMetasForTransferHookForPool
Tasks
with emit! for replay-abilitywith Memo (to avoid log truncation)The following task will be completed in the different PR. Now I'll stop pushing commits on this PR for reviewing.
Check
Detail
Upgrade to Solana 1.17 / Anchor 0.29
use of test.genesis at Anchor.toml
I Changed to the method of loading downloaded binaries.
Cloning the TokenMetadata program with
test.validator.clonefailed to load the TokenMetadata program because the data account has been cloned withexecutable = false.In my opinion, this is a bug of Anchor (not critical).
use of anchor_spl::metadata
mpl_token_metadatav1.7 needs to be upgraded due to dependency on old spl-token lib.Since
anchor_splincludesmpl_token_metadata, we useanchor_spl::metadata.Since the version of mpl_token_metadata changes frequently, use the one included in anchor_spl to avoid dependency issues.
updates due to breaking of Anchor
https://github.com/coral-xyz/anchor/blob/master/CHANGELOG.md
account(zero_copy(unsafe))feature. coral-xyz/anchor#2330)To keep the current behaviour, we need to use
zero_copy(unsafe).use anchor 0.9.1 as
borsh09in Cargo.tomlTo avoid conflict between 0.9 and 0.10 used in Anchor, give alias for v0.9.1.
FreezeAuthority and Supported extensions
Freeze Authority
Recently dysfunctional pools have emerged as a result of FreezeAuthority being used (COCO/X pools).
initialize_pool_v2 and initialize_reward_v2 reject Token-2022 program tokens with FreezeAuthority unless TokenBadge is initialized.
For Token program tokens, they are accepted even if they have FreezeAuthority for compatibility with v1.
Supported extensions
We believe that the authority to take tokens out of the pool (PermanentDelegate) is an extension that requires the extreme caution and should only be used by a limited number of stable coin issuers.
TransferHook could also disallow any transfers at all, thus it should be used by limited issuers.
MintCloseAuthority and DefaultAccountState are nonsense extensions for the pool, but the contract will accept them if TokenBadge is initialized since there are stablecoins that use them. (e.g. GYEN)
InterestBearing is not acceptable at this update.
👍Supported
🚨Supported if it has initialized TokenBadge
🚫Not Supported
TokenBadge
TokenBadge
It was determined that the ability to freely create pools with tokens that have functions that may malfunction the pools, such as PermanentDelegate, has more disadvantages for abuse than advantages. However, several stable coins already have PermanentDelegate extension.
If tokens with features such as PermanentDelegate are used to create pools or initialize rewards, some whitelist mechanism is required, which is TokenBadge.
TokenBadge is a PDA whose seed is WhirlpoolsConfig and Mint, and each WhirlpoolsConfig can control whether or not to create TokenBadge in its space.
The TokenBadge itself only records the WhirlpoolsConfig and Mint used at initialization, and has no other additional information at the moment.
The name "TokenBadge" intentionally avoids the centralized words "Whitelist," "Support," and "Allow", etc.
Related new instructions:
ConfigExtension
The addition of TokenBadge Authority is required as the authority that can manage TokenBadge.
There is no authority in WhirlpoolsConfig with a name that could serve as TokenBadge authority. However, there is no room in the account to add an additional authority without resizing it.
Therefore, to avoid size breaking, we need to add a new account called WhirlpoolsConfigExtension account and add token_badge_authority there.
WhirlpoolsConfigExtension reserves 512 bytes of additional space for future additions.
It also gives config_extension_authority modification privileges to facilitate adding new privileges to the area initialized with 0.
Since WhirlpoolsConfigExtension is updated by config_extension_authority, it is not possible to set a new token_badge_authority with the token_badge_authority authority.
We have a bit of a tricky problem of who can initialize WhirlpoolsConfigExtension. fee_authority was used as the authority that can initialize it. Also it is used as the initial value of config_extension_authority and token_badge_ authority. Once WhirlpoolsConfigExtension account is initialized, config_extension_authority is authority to manage it.
Related new instructions:
TransferHook support
use of remaining_accounts
The account used for TransferHook is different for each program used by TransferHook. Therefore, they are received through
remaining_accounts.To clarify the context of accounts passed as
remaining_accounts, the v2 instructions receiveremaining_accounts_infoas data. It is used to classify the accounts contained inremaining_accounts. (e.g. The first 3 are for mintA's TransferHook, next 2 are for for mintB's TransferHook)transfer-hook-counter
A simple hook program, which count transfers, have been added to
sdk/tests/external_program.It is used in test only.
Its code is here: https://github.com/yugure-orca/transfer-hook-counter/tree/main/programs/transfer-hook-counter
TransferFee support
amount and threshold
The relationship between
amount,otherAmountThresholdand TransferFee in each context.ExactIn (
amount_specified_is_input = true)amount: transfer fee Included amount (will be sent from user's token account)otherAmountThreshold: transfer fee Excluded amount (will be received at user's token account)ExactOut (
amount_specified_is_input = false)amount: transfer fee Excluded amount (will be received at user's token account)otherAmountThreshold: transfer fee Included amount (will be sent from user's token account)The user can set conditions on the amount actually going out of and coming into the token account, regardless of the amount of fees.
No additional parameters are added to limit the amount of fees.
If the fee will be changed, it is forced to be activated two epochs after, so there is no possibility of the fee suddenly going up. The edge case is when the fee change is scheduled (new epoch is coming) in less than transaction life time. In this case, the UI may need to alert users, but in any case, transactions that exceed the outgoing and incoming amount thresholds will fail.
minA/B, maxA/B
Increase Liquidity:
maxA, maxBtransfer fee Included amount (will be sent from user's token account)
Decrease Liquidity:
minA, minBtransfer fee Excluded amount (will be received at user's token account)
memo applied TransferFee config
When transferring tokens has TransferFee extension, the contract uses Memo program to record the rate (bps) and cap (maximum).
msg!andemit!have the risk that logs are truncated, so Memo program is used. I believe that the ability to analyze and reproduce transactions without tracking the TransferFeeConfig state of each Mint would be a great advantage. Downside is increase of compute units.MemoTransfer support
If user token account require "memo", the following message will be used.
https://github.com/orca-so/whirlpools/pull/134/files#diff-a1e6ca856a625766f4d07239347cac8c85365c7ee34193b87b00a882032dc32a
decrease liquidity is recorded as "Withdraw" and swap is recorded as "Trade".
The message is intended to benefit the user rather than being strongly implementation-dependent.
Additional Notes
add constraint to force tickSpacing / FeeTier match
Prevent the use of tickSpacing where FeeTier is not initialized. (v1 & v2)
https://github.com/orca-so/whirlpools/pull/134/files#diff-49637519268956e8e45d1f6a84d412a5ca421e764b98a50332e8c39878d8226cR42
set mut on oracle accounts
swap_v2andtwo_hop_swap_v2setmuton oracles for future implementation without breaking.WSOL-2022 is not supported
In Token-2022 program, another native mint (
9pan9bMn5HatX4EJdBwg9VgCa7Uz5HL8N1m5D3NdXejP) is defined.In this update, WSOL-2022 is rejected as unsupported mint because we want to avoid SOL liquidity fragmentation.
TwoHopSwap v2 uses vault to vault transfer
In two_hop_swap, the intermediate token is sent once to the user's account, and then sent from the user's account to the vault in the second pool. This was safe because the swap operation was simply performed twice.
In two_hop_swap_v2, the intermediate token is sent directly from the first pool to the second pool.
This is to avoid incurring double fees on tokens with TransferFee.
Cases where the output of the first swap mismatches with the input of the second swap should be an error.
SDK Update
Deprecation
TokenExtension is not supported in the deprecated functions.
The following functions have been marked as deprecated.
These function requires so many Whirlpool and TickArray accounts. The number of Whirlpool accounts is increasing rapidly, so it is no longer possible to fetch all of them to calculate best route and robust price at client side.
Breaking Changes