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

Add 'takeoffer' api method #4667

Closed
wants to merge 12 commits into from
Closed

Conversation

ghubstan
Copy link
Contributor

@ghubstan ghubstan commented Oct 18, 2020

This is the first trade protocol method implementation.

#4666 needs to be reviewed and merged before this PR.

The changes needing the most scrutiny are in the new bisq.core.offer.takeoffer.TakeOfferModel, which duplicates some logic in bisq.desktop.main.offer.takeoffer.TakeOfferDataModel that could not be moved into :core due to JFX dependencies.

Changes include:

  • Adding new Trades service to grpc.proto.

  • Adding new CoreTradeService and GrpcOffersService.

  • Adding two utility methods to OfferUtil, instead of including them in TakeOfferModel.

  • Adding light proto wrapper bisq.core.api.model.TradeInfo.

  • Moving static toOfferInfo from GrpcOffersService to OfferInfo.

  • Adding takeoffer to CliMain.

  • Adding new TakeOffer test and some refactoring of offer test classes.

Additional api methods to implement trade protocol steps and check trade status will be added in some of the next PRs.

This is a preliminary refactoring for a new core TakeOfferModel, which
is going to need logic (for api 'takeoffer') currently located in the
unaccessible desktop module's offer package.

This change injects service dependencies into the OfferUtil instance and
removes those service dependency arguments from formerly static method
signatures.

Other changes include:

- Moving some Volume calc methods from OfferUtil to VolumeUtil.

- Moving some Coin calc methods from OfferUtil to CoinUtil.

- Injecting OfferUtil into CreateOfferService and relevant
  offer data & view models.

- Minor changes to TradeStatistics2, Trade, Offer & Contract, to use
  static methods moved from OfferUtil to the new VolumeUtil class.

- Deletion of bisq.desktop.main.offer.MakerFeeProvider,
  and bisq.core.offer.MakerFeeProvider.  They were unnecessary
  wrappers around a single OfferUtil method.

- Renaming OfferUtilTest -> CoinUtilTest.

- Moving tests in CoinCryptoUtilsTest to CoinUtilTest, and deleting
  CoinCryptoUtilsTest.

- Adjusting affected offer data & view model tests.
This is the second prelimiary refactoring to prepare for a new
core.api takeoffer impl.  This effort is restrained by the desktop
models' dependency on JFX properties, which could not be passed
as arguments to the core OfferUtil.

Changes include:

- Moving utility logic into OfferUtil from OfferDataModel and
  MutableOfferDataModel.

- Injecting OfferUtil into TakeOfferDataModel, for parent OfferDataModel.

- Removing unused TradeManager from OfferBook.

- Moving 'isHalCashAccount' acct type check to abstract PaymentAccount.

- Moving 'getTradeCurrency' logic to abstract PaymentAccount from
  MutableOfferDataModel's 'setTradeCurrencyFromPaymentAccount',
  and changing affected classes:  MutableOfferView, MutableOfferViewModel.
This is the first trade protocol method implementation.

The changes needing the most scrutiny are in the new
bisq.core.offer.takeoffer.TakeOfferModel, which duplicates
logic in bisq.desktop.main.offer.takeoffer.TakeOfferDataModel
that could not be moved into :core due to JFX dependencies.

Changes include:

- Adding new TradesService to grpc.proto.

- Adding new CoreTradeService and GrpcOffersService.

- Adding two utility methods to TakeOfferModel (instead of including
  them in TakeOfferModel).

- Adding light proto wrapper bisq.core.api.model.TradeInfo (for CLI).

- Moving static OfferInfo toOfferInfo from GrpcOffersService to OfferInfo.

- Adding new CoreTradeService

- Adding 'takeoffer' method to CliMain.

- Adding new TakeOffer test and some refactoring of offer test classes.
Copy link
Contributor Author

@ghubstan ghubstan left a comment

Choose a reason for hiding this comment

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

Commit 48c851d msg has error Adding two utility methods to TakeOfferModel (instead of including
them in TakeOfferModel)
.

Should be Adding two utility methods to OfferUtil (instead of including
them in TakeOfferModel).

@ghubstan
Copy link
Contributor Author

Closing, too many file conflicts in this PR chain, after 1.4.x merge into main, will redo.

@ghubstan ghubstan closed this Oct 20, 2020
@ghubstan ghubstan deleted the takeoffer-3 branch October 22, 2020 15:16
@ripcurlx ripcurlx added this to the v1.5.2 milestone Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants