-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: lp_subscribe_order_fills (PRO-938) #4319
Conversation
db0b117
to
96c619a
Compare
chore: snake case for ApiWaitForResult refactor: factor out wait for result unwrapping refactor: use submit_signed_xt inside the wait_for call feat: withdraw asset wait for refactor: wait for is optional chore: use txhash and txdetails naming
@dandanlen Yeah sorry I totally forgot about the old PR, but your comments have been addressed. The old PR was signficantly broken. |
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.
Just some surface level comments for now, I can take a closer look later if necessary.
api/bin/chainflip-lp-api/src/main.rs
Outdated
hash: Hash, | ||
number: BlockNumber, |
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:
block_hash: Hash,
block_number: BlockNumber,
?
I think this would be a little clearer.
id, | ||
.. | ||
}) => { | ||
Some((lp.clone(), AssetPair::new(*base_asset, *quote_asset).unwrap(), *id)) |
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.
Presumably the unwrap is ok because we know it was constructe from a valid base/quote pair when the event was constructed?
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.
yes
The unwraps are annoying, but the behaviour is correct. This behaviour matches other subscriptions in substrate.