Add API 'gettrades' method #5976
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This PR remains in draft status until I figure out how to display the correct share of the BSQ Swap's tx-miner-fee paid by the swap offer maker and taker. The console is incorrectly displaying the full tx-miner-fee for maker and taker. |
ripcurlx
reviewed
Jan 25, 2022
|
@ghubstan I just tried out the gettrades endpoint and it failed for me with following error when trying to call: |
|
When trying to call It failed with: I do know that I have quite a couple of nasty failed trades in my list, but I think this should not prevent this request to execute successfully. |
ghost
suggested changes
Jan 25, 2022
cli/src/main/java/bisq/cli/table/builder/AbstractTradeTableBuilder.java
Outdated
Show resolved
Hide resolved
|
Tested again the new gettrades endpoints: @ghubstan Everything seems to work now for my data set, but I think the response list should be sorted by date. BTW. do we have any limits option in our API endpoints regarding the max response entries? Might not be super critical ATM, but this could change in the future if liquidity changes and the amount of offers online. WDYT? |
ghost
previously approved these changes
Jan 26, 2022
By the way amigo, that's some baaaaad data ;-) It is very useful. |
|
It seems to have worked for failed trades: but not for closed trades (canceled trades are not included in sorting): For open trades it also seems to work (have to check with more open trades): |
ripcurlx
reviewed
Jan 27, 2022
| @@ -292,7 +298,10 @@ Trade getTrade(String tradeId) { | |||
| List<TradeModel> getOpenTrades() { | |||
| coreWalletsService.verifyWalletsAreAvailable(); | |||
| coreWalletsService.verifyEncryptedWalletIsUnlocked(); | |||
| return tradeManager.getTrades().stream().map(t -> (TradeModel) t).collect(Collectors.toList()); | |||
| return tradeManager.getTrades().stream() | |||
| .map(t -> (TradeModel) t) | |||
There was a problem hiding this comment.
Why do you need to map it here?
There was a problem hiding this comment.
It is something @xyz123 asked for in his review: "why isn't the mapping consistent between return types for List<TradeModel> getOpenTrades() and List<TradeModel> getTradeHistory(...)"? I did intend make the return types consistent, but overlooked it.
There was a problem hiding this comment.
I don't mean so much about the return types of the method, but rather the fact, that in this case it is unnecessary to cast.
There was a problem hiding this comment.
I removed two casts in 8206425, but the mapping in line 303 had to stay.
ripcurlx
reviewed
Jan 27, 2022
| } else { | ||
| var failedV1Trades = failedTradesManager.getTrades(); | ||
| return failedV1Trades.stream().map(t -> (TradeModel) t).collect(Collectors.toList()); | ||
| return failedV1Trades.stream() | ||
| .map(t -> (TradeModel) t) |
There was a problem hiding this comment.
Why do you need to map it here?
|
Sorting for closed trades does not work as you build the final response list in |
You are right. Fix will come later today. |
This reverts commit f231aac. Sorting cumulative trade history lists should be done once, in GrpcTradesService.
See 111e39f. |
ripcurlx
approved these changes
Feb 1, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
gettradesmethod takes an--category=[open|closed|failed]parameter and displays pending, closed, and failed trades, respectively:Based on
master.