-
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
Add core math packages (using wasm) #201
base: main
Are you sure you want to change the base?
Conversation
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.
few nits but looks good!
rust-sdk/utils/src/math/tick.rs
Outdated
mul.wrapping_shl(96).try_into().unwrap() | ||
} | ||
|
||
fn get_sqrt_price_positive_tick(tick: TickIndex) -> u128 { |
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.
might be good to link to where these constants come from and/or describe the overall algorithm in a comment
15185d6
to
acc72f8
Compare
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.
this is looking good my only thought is to use checked_div
instead of /
so we can avoid any unintended divide-by-zero errors
let amount_a = upper_sqrt_price - current_sqrt_price; | ||
let amount_total = amount_a + amount_b; | ||
|
||
let ratio_a = (amount_a * 10000) / amount_total; |
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.
div by zero check here?
@yugure-orca I think this now contains all the core math and quoting logic for whirlpools 🥳. I am however not 100% confident yet that all the logic is implemented correctly 😬. Especially some of the round ups and downs when doing when doing muldiv and bitshifts As stated this same logic is exported through wasm (still with std lib but we can fix that later) so rust sdk and ts sdk will share the exact same logic 🎊 (If in the future we ever want to make sure that the core logic is shared between the SDKs and the program that should theoretically be possible with the way I set up the core package) |
20a1a9d
to
a11aeff
Compare
# Conflicts: # yarn.lock
rust-sdk/core/src/math/tick_array.rs
Outdated
} | ||
|
||
impl TickArraySequence { | ||
pub fn new(one: TickArrayFacade, two: TickArrayFacade, three: TickArrayFacade, tick_spacing: u16) -> 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.
swap v2 takes in additional tick-arrays via the remaining_accounts. So potentially we can support n-many tick-arrays here (limited by compute units / tx-size). I believe the tick-arrays can also be uninitialized with sparse-swap.
1st iteration is fine but a heads up we may want to take in a Vec for the current swap-quote & have a variant of swap-quote that hides the complexity of finding the right tick-array sequence.
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.
From what I understand in the new swap instruction 3 TA can be specified directly with the option to add up to 3 more through remaining_accounts
. The swap instruction itself however will only ever use 3 TAs at max
# Conflicts: # docs/ts/package.json # yarn.lock
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.
lgtm, final versions of the macros look sound and well tested
Add two new packages to the monorepo
This approach is great because this allows us to have a single source of truth for math functions that are used throughout other libs (and potentially even the program). Writing it once means that the implementations will always be identical regardless of which platform you are on. Also means we only have to write tests for them once
The wasm lib currently only builds with std enabled because of something in
tsify
. This inflates the wasm bundle by quite a bit. I opened a ticket: madonoharu/tsify#56. This only affects thets-sdk/core
and is a bundle size thing so not the end of the world. All code behaves identically as if it was built with/without stdThis PR only contains basic test cases and does not test for any of the more tricky / edge cases for swap quoting. My brain is fried from looking at tick array traversal and bit math the past couple of days so I'd like to continue with other parts of the SDK and revisit this later.
note: Since this is not added to
publish
workflow yet this will not be deployed yet on tag