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

Add getRole(tradeId) to core api #4701

Merged
merged 10 commits into from
Oct 30, 2020
Merged

Add getRole(tradeId) to core api #4701

merged 10 commits into from
Oct 30, 2020

Conversation

ghubstan
Copy link
Contributor

API users will need to see their role as maker/taker when looking at trade details.

  • Add getRole(trade) to TradeUtil
  • Add getTradeRole(tradeId) to CoreApi, CoreTradesService
  • Add role field to TradeInfo proto its wrapper class

PR #4700 should be reviewed & merged before this PR.

ghubstan added 10 commits October 23, 2020 12:47
@ghubstan
Block on tx-fee-request in core TakeOfferModel init
cb65de6 
Added license comment too.
@ghubstan
Add compiler warning suppression, remove comment
ab20225
@ghubstan
Add fields to grpc TradeInfo proto & wrapper
63cf436
@ghubstan
Replace static TradeUtil with singleton TradeUtil
296e4f9 
The API is going to need some desktop trade utilities, which should be
shared between :desktop and :core.api.
@ghubstan
Fix comment typos
ccd3c99
@ghubstan
Refactor PendingTradesViewModel methods -> TradeUtil & OfferUtil
24ba921
@ghubstan
Remove trailing spaces for codacy
36ad137
@ghubstan
Refactor PendingTradesDataModel methods -> TradeUtil
95bcb1e 
Scope of this refactoring is small;  more can be done, but the short
term goal is to share trade util logic with core api.

- Removed unused method getCurrencyCode()
- Made minor style changes
- Removed duplicated code block
@ghubstan
Remove unused import
bbd7a31
@ghubstan
Add getRole(tradeId) to core api
161dbad 
API users will need to see their role as maker/taker when looking at trade details.

- Add getRole(trade) to TradeUtil.
- Add getTradeRole(tradeId) to CoreApi, CoreTradesService.
- Add role field to TradeInfo proto and its wrapper class.
@ghubstan ghubstan mentioned this pull request Oct 25, 2020
Closed
@ghubstan
Copy link
Contributor Author

This does not add a getrole api method to the CLI, it just makes the user's trade role available within the output from a gettrade trade-id call. Future trade utils refactoring may make this cleaner, and push the coreApi.getRole call down & out of GrpcTradesService.

@ghubstan ghubstan mentioned this pull request Oct 25, 2020
Merged
sqrrm
sqrrm approved these changes Oct 30, 2020
View reviewed changes
Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

utACK

Some comments but nothing blocking merging, just stuff to consider.

core/src/main/java/bisq/core/trade/TradeUtil.java Show resolved Hide resolved
core/src/main/java/bisq/core/trade/TradeUtil.java Show resolved Hide resolved
proto/src/main/proto/grpc.proto Show resolved Hide resolved
@sqrrm sqrrm merged commit 3c56268 into bisq-network:master Oct 30, 2020
@ghubstan ghubstan deleted the 5-refactor-trade-utils branch November 2, 2020 15:07
@ghubstan ghubstan mentioned this pull request Nov 24, 2020
Closed
@sqrrm sqrrm mentioned this pull request Nov 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sqrrm sqrrm sqrrm approved these changes

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
@ghubstan @sqrrm