-
Notifications
You must be signed in to change notification settings - Fork 58
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
[PY] [CLOB-894] [CLOB-895] v0.6.8
Quick python example for canceling short term and long term orders
#42
Conversation
'ETH-USD', | ||
ORDER_FLAGS_LONG_TERM, | ||
good_til_time_in_seconds=120, | ||
good_til_block=0, # long term orders use GTBT |
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.
why not just put GTBT here instead of GTB if the example is for long term order?
short_term_client_id, | ||
'ETH-USD', | ||
ORDER_FLAGS_SHORT_TERM, | ||
good_til_time_in_seconds=0, # short term orders use GTB. |
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.
similarly why not just put GTB here instead of GTT if the example is for short term orders
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.
Both function arguments are required it's a design choice, if we want we can make the arguments optional and add validation that correct params provided but I don't think it's a big deal
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 so you need BOTH GTB and GTT?
My read was that you would put good_til_block if it's short term and good_til_Time_in_seconds if it's long term... is that correct? if so, then since this example is the "short term" example why don't we just make it "good_til_block" here instead of "good_til_time_in_seconds" and then having a comment that says short term should be GTB
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.
yeah you need both but zero out the one you don't need. Definitely agree with Yi but this can be fixed later, want to get something up and working
market='ETH-USD', | ||
side=OrderSide.SELL, | ||
price=40000, | ||
size=0.01, |
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.
is this in base quantums
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.
No, the concept of base quantums is not exposed to client users
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 so just USDC
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.
No it's in base coins, eg bitcoin
Price is defined in USDC / coin
# Get the expiration block. | ||
current_block = client.get_current_block() | ||
next_valid_block_height = current_block + 1 | ||
# Note, you can change this to any number between `next_valid_block_height` to `next_valid_block_height + SHORT_BLOCK_WINDOW` |
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.
which is just current block + 20 right?
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.
# place a long term order. | ||
long_term_order_client_id = randrange(0, 2 ** 32 - 1) |
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.
Can we split this into a separate example? I don't want any MMs onboarding to testnet to get confused about how to place a Short-Term order vs stateful order, and ideally we can just send them one example.
@@ -106,30 +117,30 @@ def calculate_time_in_force( | |||
time_in_force: OrderTimeInForce, | |||
execution: OrderExecution, | |||
post_only: bool |
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.
Why is this taken as an argument when it's defined in the execution
param?
This function is really confusing, ideally we can simplify it since it's not used in the FE and therefore has no dependencies
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.
@johnqh ?
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 don't think i'll touch this to preserve correctness since I'm not entirely familiar with this function. Seems to be related to how we translate FE calls to our chain's proto fields.
time_in_force=OrderTimeInForce.GTT, | ||
good_til_time_in_seconds=60, | ||
execution=OrderExecution.DEFAULT, | ||
post_only=False, |
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.
Any way we can remove the post_only
arg in favor of using execution
?
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.
price=40000, | ||
size=0.01, | ||
client_id=long_term_order_client_id, | ||
time_in_force=OrderTimeInForce.GTT, |
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.
Can we just remove this completely and have the client provide order flags instead?
It's really confusing that order flags are automatically inferred from the TimeInForce
, IMO client should explicitly choose what order flags to use.
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.
Punting this to get something functional out. This PR will be for cancels, which take in the explicit order flag, which user provides in cancel_order
.
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.
Do you need to increment PyPi package version?
8977766
to
883f5a6
Compare
v0.6.8
Quick python example for canceling short term and long term orders
also fix #38