openbook-dex / openbook-v2

openbook-v2 monorepo, contains solana program and ts client
Other
153 stars 84 forks source link

cancel by client_id iterates and fails silently #237

Closed mschneider closed 6 months ago

mschneider commented 6 months ago

cancel by client id previously would fail the transaction making it very difficult to implement cancelling a subset of orders (e.g. assigned to an individual trading engine inside a multi strategy fund). if an order is filled while the users transaction is inflight, a direct cancel with a client id can fail if there is no such order.

in my opinion this version is better, it is easier to use

also removed the limit on prune orders, as the OO account is anyways limited to 24 orders, there's not a lot of value in an explicit limit argument.

farnyser commented 6 months ago

Seems weird to me to have a different behavior for cancel_by_id (=> fail) and for cancel_by_client_id (=> skip).

Also I find it nice to be able to abort a TX if the client view of the market doesn't match the current state. Maybe we should add a flag similar to the SelfTradeBehaviour to select whether to fail the TX or skip the IX ?

mschneider commented 6 months ago

this change was requested from a market maker, client id is mainly used by bots that have to work around read back latency while order id is used by end users and cpi clients which dont have that issue

Henry-E commented 6 months ago

@mschneider Changing the prune function like this is presumably a breaking change for anyone with a program calling prune (e.g. the meta openbook-twap program). Not sure how often you push updates but please reconsider leaving this breaking change for a more major version update. Especially since it's so minor.

mschneider commented 6 months ago

Can put it back in, didn’t know anyone uses this yet

On Mon 25. Mar 2024 at 12:09, Henry-E @.***> wrote:

@mschneider https://github.com/mschneider Changing the prune function like this is presumably a breaking change for anyone with a program calling prune (e.g. the meta openbook-twap program). Not sure how often you push updates but please reconsider leaving this breaking change for a more major version update. Especially since it's so minor.

— Reply to this email directly, view it on GitHub https://github.com/openbook-dex/openbook-v2/pull/237#issuecomment-2017750936, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABDF7AGRLQG2KXSLSJ4QCTY2AAWTAVCNFSM6AAAAABEZ3XCSKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJXG42TAOJTGY . You are receiving this because you were mentioned.Message ID: @.***>

Henry-E commented 6 months ago

Awesome, thanks. Appreciate it