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

RFQ cleanup #860

Merged
merged 4 commits into from
Apr 2, 2024
Merged

RFQ cleanup #860

merged 4 commits into from
Apr 2, 2024

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Mar 28, 2024

This PR was recently reviewed, approved, and merged: #835

I noticed that some of the review comments could also apply to other parts of the RFQ code that were not changed in the above PR.

This PR improves the RFQ code following the review comments from #835.

@ffranr ffranr requested review from jharveyb and GeorgeTsagk March 28, 2024 12:53
@dstadulis dstadulis added this to the v0.4 milestone Mar 28, 2024
ffranr added 3 commits March 28, 2024 15:47
In this commit we replace methods `encodeRecords` and `decodeRecords`
with a single method called `records`.
Function `wait.NoError` will call the given target function multiple
times. Whereas `BeforeTimeout` calls the given target function once.
We don't want to call the target function more than once in this itest.
This commit changes `wait.NoError` calls to `BeforeTimeout`.
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

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

Looks good!

rfqmsg/buy_accept_test.go Show resolved Hide resolved
@ffranr ffranr added this pull request to the merge queue Apr 2, 2024
Merged via the queue into main with commit 2affd4e Apr 2, 2024
14 checks passed
@guggero guggero deleted the rfq-cleanup branch April 2, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants