-
Notifications
You must be signed in to change notification settings - Fork 293
Implements resetPositionRange instructions for ts-sdk #943
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
Conversation
| } | ||
| }); | ||
|
|
||
| const testOpenPositionInstructions = async ( |
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.
what I intend that
- creating a new position
- remove all liquidity
- and reset position range, and check if position is correctly changed.
wjthieme
left a comment
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.
Think we will also need this for the rust-sdk (or will you handle that in a separate PR?
Yes, there are many instructions that are not implemented in rust-sdk, such as |
1ce9508 to
721e409
Compare
a674c51 to
3664a46
Compare
odcheung
left a comment
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.
generally looks good! but i want to push for more unit-tests
| ); | ||
|
|
||
| instructions.push( | ||
| getResetPositionRangeInstruction({ |
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.
where is this method defined? I don't see it in HEAD or elsewhere
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.
getResetPositionRangeInstruction is defined at @orca-so/whirlpools-client.
| for (const positionTypeName of positionTypes.keys()) { | ||
| const positionName = `${poolName} ${positionTypeName}`; | ||
| it(`Should reset a position for ${positionName}`, async () => { | ||
| await testResetPositionRange(poolName, positionName); |
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.
we also need to verify that the reset position ix will fail if the position is not empty and if the user is trying to set it to the same tick range.
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.
Agree, added more tc for those cases.
5028be2 to
70d439e
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.
lgtm - but if we can modify the test cases to use test.for instead of a custom loop that'd be great
| for (const poolName of poolTypes.keys()) { | ||
| for (const positionTypeName of positionTypes.keys()) { |
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'm a little bummed that vitest doesn't have .each functions where we can declare test variations on like .each on jest!
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.
wait. seems like it does?
https://vitest.dev/api/#test-for
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.
oh I am checking :D
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 created a new ticket for this, and will fix this
This PR implements
resetPositionRangeinstructions in typescript-sdk