-
Notifications
You must be signed in to change notification settings - Fork 135
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
SparseSwap #159
SparseSwap #159
Conversation
program can use up to 32KB heap
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.
Took a pass over the core logic code, will do another pass over edge cases and tests
programs/whirlpool/src/state/tick.rs
Outdated
} | ||
|
||
pub fn check_in_array_bounds(&self, tick_index: i32, tick_spacing: u16) -> bool { | ||
self.in_search_range(tick_index, tick_spacing, false) | ||
} | ||
|
||
pub fn is_min_tick_array(&self) -> bool { | ||
self.start_tick_index <= MIN_TICK_INDEX | ||
is_min_tick_array(self.start_tick_index) |
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.
nit: I think it'd be possible to define a trait that could help de-duplicate shared code between ZeroedTickArray
and TickArray
, for example,
pub trait TickArrayType {
fn start_tick_index();
fn is_min_tick_array(&self) -> bool {
is_min_tick_array(self.start_tick_index())
}
fn is_max_tick_array(&self, tick_spacing: u16) -> bool {
is_max_tick_array(self.start_tick_index(), tick_spacing)
}
...
}
impl TickArrayType for TickArray {
fn start_tick_index(&self) -> i32 {
self.start_tick_index
}
}
impl TickArrayType for ZeroedTickArray {
fn start_tick_index(&self) -> i32 {
self.start_tick_index
}
}
(could even more the shared functions into the trait, since they aren't being called externally anyways)
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.
cool! 🚀
updated with TickArrayType
trait.
} | ||
|
||
pub fn start_tick_index(&self) -> i32 { | ||
match self { |
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.
nit: I think if we use the above trait approach, I think we can simplify the code here a little bit by implementing a method such as
impl<'a> AsRef<dyn TickArrayType + 'a> for ProxiedTickArray<'a> {
fn as_ref(&self) -> &(dyn TickArrayType+ 'a) {
match self {
ProxiedTickArray::Initialized(ref array) => &**array,
ProxiedTickArray::Uninitialized(ref array) => array,
}
}
}
Which would enable us to write something like
pub fn start_tick_index(&self) -> i32 {
self.as_ref().start_tick_index()
}
Instead of having to have matches in each function
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.
cool again 👍
updated with AsRef & AsMut.
TickArrayAccount::Initialized { account_info, .. } => { | ||
use std::ops::DerefMut; | ||
|
||
let data = account_info.try_borrow_mut_data()?; |
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.
hmm...is there a significance to dropping the original mapping in peek_tick_array
https://github.com/orca-so/whirlpools/pull/159/files#diff-4f07d33d8b2d8286ff9afe2fc286ae78c27391485b7163c8ac30490e7d9139bbR299 only to re-deserialize it here?
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.
good point!
In my understanding, it is just memory mapping operation (pointer operation in low-level), so no significant cost.
And I think we've already done this processing (load & load_mut).
https://github.com/orca-so/whirlpools/blob/main/programs/whirlpool/src/instructions/two_hop_swap.rs#L44
But I agree that it is nice to merge try_from
and build
and dedup this logic.
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.
I tried for a while to merge try_from
and build
, but the lifetime issue is troublesome, so I would like to leave it as it is.
}) | ||
} | ||
|
||
pub fn build<'a>(&'a self) -> Result<SwapTickSequence<'a>> { |
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.
nit: I believe in this PR all calls to SparseSwapTickSequenceBuilder::try_from
are immediately followed up by .build
, is there a reason to not have a combined call for callers?
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.
Initially, a dummy TickArray was allocated in the heap; it was necessary to maintain ownership of the heap in order to use RefMut.
The Builder was more significant for not cluttering up the handler with variables for this heap than for its logic.
However, ZeroedTickArray probably doesn't need to do this anymore.
will try to refactor.
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.
I tried for a while to merge try_from
and build
, but the lifetime issue is troublesome, so I would like to leave it as it is.
whirlpool: &Account<'info, Whirlpool>, | ||
a_to_b: bool, | ||
static_tick_array_account_infos: Vec<AccountInfo<'info>>, | ||
supplemental_tick_array_account_infos: Option<Vec<AccountInfo<'info>>>, |
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.
nit: I know you added this in the PR comment, but it would be good to describe the logic here, specifically that the SparseSwapTickSequenceBuilder
will still cap the SwapTickSequence to 3
tick arrays, even if 6 are passed in
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.
Agreed.
Added comments.
} | ||
} | ||
|
||
let start_tick_indexes = get_start_tick_indexes(whirlpool, a_to_b); |
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.
nit: Do we know the relative cost of loading an account vs deriving_tick_array_pda
? If the computational cost of derive_tick_array_pda
is significantly less than loading the account, then it might be good to filter the tick_array_account_infos
by the PDAs prior to loading the account.
Either way, might be good to comment on why we choose the ordering that we do, i.e. load accounts -> filter by start index/PDA
vs filter by start index/PDA -> load accounts
.
Another optimization could be to change based on how many accounts there are in tick_array_account_infos
as well
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.
Added comment.👍
PDA calculation is a high cost process that costs 3000 CU/PDA ~, so if it can be matched against start_tick_index, it is preferred.
fn peek_tick_array<'info>(account_info: AccountInfo<'info>) -> Result<TickArrayAccount<'info>> { | ||
use anchor_lang::Discriminator; | ||
|
||
// following process is ported from anchor-lang's AccountLoader::try_from and AccountLoader::load_mut |
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.
nit: might be good to give a reason why we want to port this logic rather than using AccountLoader directly, i.e. performance, more granularity so that we don't fail on uninitialized accounts, etc
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.
It is difficult to satisfy the reference condition required by AccountLoader::try_from.
Added comment as to why.
Probably related issue: coral-xyz/anchor#2770
|
||
fn is_valid_start_tick_index(start_tick_index: i32, ticks_in_array: i32) -> bool { | ||
// caller must assure that start_tick_index is multiple of ticks_in_array | ||
start_tick_index + ticks_in_array > MIN_TICK_INDEX && start_tick_index < MAX_TICK_INDEX |
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.
hmm, I think there is a discrepancy between this and the existing check_is_valid_start_tick
, I believe the existing check_is_valid_start_tick
uses an equality condition on theMIN_TICK_INDEX
, i.e. start_tick_index + ticks_in_array >= MIN_TICK_INDEX
?
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.
Good catch!
will check this edge case.
Good news is that 443636 (abs value of MIN_TICK_INDEX) is not multileple of 88.
So this MIN_TICK_INDEX will never be start_tick_index. 😁
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.
I chose the simplest approach.
Use Tick::check_is_valid_start_tick
😁
merged FROM the latest main to resolve conflict on errors.rs. |
* intro the benefit of SparseSwap into SDK * avoid breaking parameter change * fix failed test cases * adjust fallback tickarray handling * fix typo * add new test cases * add test cases for fallback * address review comment (fix typo)
The audit for 3f30966 have been completed. |
Problems
Uninitialised TA problem
Problems with the order of TickArrays passed to instructions
It is necessary to provide a TickArray containing
tick_current_index
astick_array_0
.However, if there is a current price near the boundary of the TickArray, this condition may not be met because the price moves; it is a kind of slippage, but even if the slippage is increased, an error occurs in this case.
Solution
Accept uninitialized TickArray
swap, swapV2, twoHopSwap, twoHopSwapV2 will accept uninitialized TickArray account address.
These instructions will treat uninitialized TickArray account as if they are zero padded and process trade.
On-chain TickArray account order adjustment
swap, swapV2, twoHopSwap, twoHopSwapV2 will accept (maybe uninitialized) TickArray account address in any order and sort them as needed. If instruction receive required TickArray accounts, it should work.
Additional TickArray accounts
swapV2 and twoHopSwapV2 will accept additional up to 3 TickArray accounts via remaining_accounts.
Implementation Notes
No breaking change on instruction interface
Switching AccountLoader to UncheckedAccount
AccountLoader can only accept accounts that have been initialized (or at least the owner program has been changed to initialize account).
Therefore, to receive uninitialized accounts, use UncheckedAccount.
The verification that was conventionally done via has_one needs to be done via handler.
Also, the verification done internally by load_mut() needs to be done the same way.
ProxiedTickArray
Add one abstraction layer, ProxiedTickArray, to minimize changes to the current battle-tested
SwapTickSequence
code.For TickArrays that have been initialized, the process is delegated to the existing implementation. For uninitialized TickArray, the process is delegated to ZeroedTickArray where all Tick are zero initialized.
Notes: Heap size limination
Initially, we considered placing the entire 0-initialized TickArray account in the Heap and using RefMut to that account, but the Heap size limit is 32 KB, so we were limited to two accounts at most.
Since twoHopSwapV2 handles a maximum of six uninitialized accounts, this method could not be used.
On-chain TickArray account order adjustment
First, calculate up to three start_tick_indexes for the required TickArray and verify if a TickArray account with that start_tick_index is provided.
With this method, if an account is provided that has been initialized as it is now, no PDA calculation or pubkey comparison occurs, since matching is possible only by comparing the start_tick_indexes among themselves.
The PDA calculation is only performed if no account with start_tick_index is found among the initialized accounts.
Uninitialized accounts are verified as uninitialized.
The conditions for uninitialization are that SystemProgram is the owner and the data size is 0.
Whether the uninitialized account is a correct TickArray address or not is not verified because it is impossible to verify (we need to find seend for them). However, it is used only if it matches the correct PDA derived using start_tick_index. So wrong accounts will be simply ignored.