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.

Sign up for GitHub

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

Jump to bottom

Test the role field of Trades #297

Closed
wants to merge 3 commits into from
Closed

Test the role field of Trades #297

wants to merge 3 commits into from

Conversation

fa2a5qj3
Copy link
Contributor

Ref: Missing trade role from getTrades() API haveno-dex/haveno#1146
Ref: Apply trade role to getTrades result haveno-dex/haveno#1147

Test with/without fix:

Jul-15 23:11:59:36 (+3489 ms)	[L0] BEFORE TEST "Do all trades contain role property"
Jul-15 23:11:59.559 [grpc-default-executor-0] INFO  h.d.g.i.CallRateMeteringInterceptor: GetTrades has been called 1 time in the last second, rate limit is 10/second 
Jul-15 23:11:59.684 [grpc-default-executor-0] INFO  h.d.g.i.CallRateMeteringInterceptor: GetTrades has been called 2 times in the last second, rate limit is 10/second 
Jul-15 23:11:59.753 [grpc-default-executor-0] INFO  h.d.g.i.CallRateMeteringInterceptor: GetTrades has been called 1 time in the last second, rate limit is 10/second 
Jul-15 23:11:59:35 (+299 ms)	[L0] Trades size: 6
Jul-15 23:11:59:35 (+0 ms)	[L0] Trade: 62002 role: XMR buyer as maker
Jul-15 23:11:59:36 (+1 ms)	[L0] Trade: 87102 role: XMR buyer as maker
Jul-15 23:11:59:36 (+0 ms)	[L0] Trade: 74839807 role: XMR buyer as maker
Jul-15 23:11:59:36 (+0 ms)	[L0] Trade: 62002 role: XMR seller as taker
Jul-15 23:11:59:36 (+0 ms)	[L0] Trade: 87102 role: XMR seller as taker
Jul-15 23:11:59:36 (+0 ms)	[L0] Trade: 74839807 role: XMR seller as taker

 PASS  src/HavenoClient.test.ts (58.061 s)
  ✓ Do all trades contain role property (302 ms)




Jul-15 23:21:32:60 (+3395 ms)	[L0] BEFORE TEST "Do all trades contain role property"
Jul-15 23:21:32.682 [grpc-default-executor-0] INFO  h.d.g.i.CallRateMeteringInterceptor: GetTrades has been called 1 time in the last second, rate limit is 10/second 
Jul-15 23:21:32.830 [grpc-default-executor-0] INFO  h.d.g.i.CallRateMeteringInterceptor: GetTrades has been called 2 times in the last second, rate limit is 10/second 
Jul-15 23:21:32.893 [grpc-default-executor-0] INFO  h.d.g.i.CallRateMeteringInterceptor: GetTrades has been called 1 time in the last second, rate limit is 10/second 
Jul-15 23:21:32:86 (+326 ms)	[L0] Trades size: 6
Jul-15 23:21:32:86 (+0 ms)	[L0] Trade: 62002 role: 
Jul-15 23:21:32:86 (+0 ms)	[L0] Trade: 87102 role: 
Jul-15 23:21:32:87 (+1 ms)	[L0] Trade: 74839807 role: 
Jul-15 23:21:32:87 (+0 ms)	[L0] Trade: 62002 role: 
Jul-15 23:21:32:87 (+0 ms)	[L0] Trade: 87102 role: 
Jul-15 23:21:32:87 (+0 ms)	[L0] Trade: 74839807 role: 

 FAIL  src/HavenoClient.test.ts (56.519 s)
  ✕ Do all trades contain role property (378 ms)

@fa2a5qj3 fa2a5qj3 requested a review from woodser as a code owner July 15, 2024 23:47
@woodser
Copy link
Contributor

woodser commented Jul 16, 2024

Instead of adding a new test for this specific scenario, better to call getTrades() after the offer is taken in executeTrade, test that the trade exists by ID, and test the role field for each trade in testTrade()

@fa2a5qj3
Copy link
Contributor Author

A problem happens, because testTrade() is called from multiple places, and the TradeInfo object returned from havenod.takeOffer() contains an empty/null role field therefore failing the test early. I'm not sure if that means takeOffer needs fixing, or the test needs adjusting.

woodser
woodser reviewed Jul 17, 2024
View reviewed changes
src/HavenoClient.test.ts Outdated
@@ -2816,6 +2821,7 @@ async function takeOffer(ctxP: Partial<TradeContext>): Promise<TradeInfo> {
}

// test trade model
HavenoUtils.log(0, "TradeInfo received from havenod.takeOffer: " + trade.getShortId() + " role: " + trade.getRole());
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for comment.

src/HavenoClient.test.ts Outdated
@@ -2877,6 +2883,13 @@ async function testTrade(trade: TradeInfo, ctx: TradeContext) {
expect(BigInt(trade.getBuyerSecurityDeposit())).toEqual(expectedSecurityDeposit - ctx.getBuyer().depositTxFee);
expect(BigInt(trade.getSellerSecurityDeposit())).toEqual(expectedSecurityDeposit - ctx.getSeller().depositTxFee);

HavenoUtils.log(0, "Trade: " + trade.getShortId() + " role: " + trade.getRole());
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for comment.

src/HavenoClient.test.ts Outdated
@@ -2600,6 +2600,11 @@ async function executeTrade(ctxP: Partial<TradeContext>): Promise<string> {
ctx.taker.balancesAfterPayout = await ctx.taker.havenod?.getBalances();
}

const trades = (await user1.getTrades()).concat(await user2.getTrades());
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this here, and here in takeOffer:

  • Get user1 trades, assert the trade is contained by id, and call testTrade() on each.
  • Get user2 trades, assert the trade is contained by id, and call testTrade() on each.

@woodser
Copy link
Contributor

woodser commented Jul 17, 2024 •
edited
Loading

A problem happens, because testTrade() is called from multiple places, and the TradeInfo object returned from havenod.takeOffer() contains an empty/null role field therefore failing the test early

Sorry I missed this. Sounds like the returned TradeInfo from takeOffer() needs fixed to return this field. This issue is a good chance to ensure the field is always correct.

@woodser
Copy link
Contributor

woodser commented Jul 17, 2024

#301 opened as an alternative and I added you as a co-author.

This API addition and test refactoring was a little tricker than expected.

@fa2a5qj3 fa2a5qj3 closed this Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@woodser woodser woodser left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fa2a5qj3 @woodser