Skip to content
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

fix(orderbook): age field is set in proper way #1851

Merged
merged 5 commits into from
Jun 9, 2023

Conversation

rozhkovdmitrii
Copy link

@rozhkovdmitrii rozhkovdmitrii commented May 31, 2023

When orderbook is requested the age is often different and the same for each order. This behavior has been solved

@rozhkovdmitrii rozhkovdmitrii added bug Something isn't working bug fixing week 2 labels May 31, 2023
@rozhkovdmitrii rozhkovdmitrii self-assigned this May 31, 2023
@rozhkovdmitrii rozhkovdmitrii linked an issue May 31, 2023 that may be closed by this pull request
Copy link
Member

@borngraced borngraced left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! just couple suggestions

mm2src/mm2_main/src/lp_ordermatch.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/lp_ordermatch.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/lp_ordermatch.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@onur-ozkan
Copy link
Member

Can you pull dev and push to this branch?

@rozhkovdmitrii
Copy link
Author

Can you pull dev and push to this branch?

updated, squashed, rebased

onur-ozkan
onur-ozkan previously approved these changes Jun 1, 2023
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like @shamardy to review this too.

Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fix! Only one important comment from me.

mm2src/mm2_main/src/lp_ordermatch/orderbook_rpc.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/lp_ordermatch/orderbook_rpc.rs Outdated Show resolved Hide resolved
($created_at: expr) => {
now_sec()
.checked_sub($created_at)
.ok_or_else(|| warn!("now - created_at: ({} - {}) caused an u64 underflow.", $base, $sub))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

u64 starts with a vowel sound

Suggested change
.ok_or_else(|| warn!("now - created_at: ({} - {}) caused an u64 underflow.", $base, $sub))
.ok_or_else(|| warn!("now - created_at: ({} - {}) caused a u64 underflow.", $base, $sub))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

.ok_or_else(|| warn!("now - created_at: ({} - {}) caused an u64 underflow.", $base, $sub))
.unwrap_or_default()
.try_into()
.expect("Expected default u64 converted to i64 without a problem")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.expect("Expected default u64 converted to i64 without a problem")
.expect("Expected default u64 to be converted to i64 without a problem")

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

onur-ozkan
onur-ozkan previously approved these changes Jun 6, 2023
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

--

non-blocker:

mm2src/mm2_main/src/lp_ordermatch.rs Outdated Show resolved Hide resolved
shamardy
shamardy previously approved these changes Jun 9, 2023
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@shamardy shamardy merged commit 3ceafb5 into dev Jun 9, 2023
@shamardy shamardy deleted the 1850-legacy-orderbook-age branch June 9, 2023 17:21
onur-ozkan pushed a commit that referenced this pull request Jun 13, 2023
When orderbook is requested the age field now returns the right age.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] legacy orderbook age field is wrong
4 participants