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

Refactor offer/trade related classes in core and desktop #4672

Merged

Conversation

ghubstan
Copy link
Member

@ghubstan ghubstan commented Oct 20, 2020

These refactoring changes are for reducing existing and potential duplication coming with the addition of new trading protocol support in the gRPC API. Some minor styling and logic simplification changes are also included.

  • Convert OfferUtil to injected singleton, and move various offer related utility methods into it.

  • Delete both MakerFeeProvider classes, which were wrappers around the same static old OfferUtil method.

  • Inject OfferUtil into CreateOfferDataModel, CreateOfferViewModel, TakeOfferDataModel, TakeOfferViewModel, MutableOfferDataModel, MutableOfferViewModel, OfferDataModel, EditOfferDataModel, EditOfferViewModel

  • Refactor TakeOfferViewModel

    Use OfferUtil, remove unused fields & methods.
    Made minor logic simplification, style and formatting changes.

  • MutableOfferDataModel

    Made minor logic simplification, style and formatting changes.

  • MutableOfferView uses new paymentAccount.isHalCashAccount().

  • MutableOfferViewModel

    Refactored to use new VolumeUtil, CoinUtil, OfferUtil.
    Removed unused fields & accessors.
    Made minor style change.

  • Refactored OfferDataModel to use new OfferUtil

  • Refactor CreateOfferService

    Inject and use OfferUtil
    Move some utility methods to OfferUtil
    Remove unused fields

  • Offer

    Refactored to use new VolumeUtil for volume calculations.
    Made stateProperty and errorMessageProperty fields private.

  • PaymentAccount

    Moved isHalCashAccount type check to this class.
    Moved getTradeCurrency logic to this class.

  • Contract, TradeStatistics2, TradeStatistics3

    Refactored to use new VolumeUtil for volume calculations.

  • Trade

    Refactored to use new VolumeUtil for volume calculations.
    Made minor logic simplification, style and formatting changes.

  • CoinUtil

    Moved some coin utility methods into this class

  • CoinUtilTest

    Moved (coin related) tests from CoinCryptoUtilsTest and OfferUtilTest
    into CoinUtilTest, and deleted OfferUtilTest, CoinCryptoUtilsTest.

  • Adjust create and edit offer tests to model refactoring

These refactoring changes are for reducing existing and potential
duplication coming with the addition of new trading protocol support
in the gRPC API.  Some minor styling and logic simplification changes
are also include.

- Convert OfferUtil to injected singleton, and move various offer related
  utility methods into it.

- Delete both MakerFeeProvider classes, which were wrappers around the same
  static old OfferUtil method.

- Inject OfferUtil into CreateOfferDataModel, CreateOfferViewModel,
  TakeOfferDataModel, TakeOfferViewModel, MutableOfferDataModel,
  MutableOfferViewModel, OfferDataModel, EditOfferDataModel,
  EditOfferViewModel

- Refactor TakeOfferViewModel

	Use OfferUtil, remove unused fields & methods.
	Made minor logic simplification, style and formatting changes.

- MutableOfferDataModel

	Made minor logic simplification, style and formatting changes.

- MutableOfferView uses new paymentAccount.isHalCashAccount().

- MutableOfferViewModel

	Refactored to use new VolumeUtil, CoinUtil, OfferUtil.
	Removed unused fields & accessors.
	Made minor style change.

- Refactored OfferDataModel to use new OfferUtil

- Refactor CreateOfferService

	Inject and use OfferUtil
	Move some utility methods to OfferUtil
	Remove unused fields

- Offer

	Refactored to use new VolumeUtil for volume calculations.
	Made stateProperty and errorMessageProperty fields private.

- PaymentAccount

	Moved isHalCashAccount type check to this class.
	Moved getTradeCurrency logic to this class.

- Contract, radeStatistics2, TradeStatistics3

	Refactored to use new VolumeUtil for volume calculations.

- Trade

	Refactored to use new VolumeUtil for volume calculations.
	Made minor logic simplification, style and formatting changes.

- CoinUtil

	Moved some coin utility methods into this class

- CoinUtilTest

	Moved (coin related) tests from CoinCryptoUtilsTest and OfferUtilTest
	into CoinUtilTest, and deleted OfferUtilTest, CoinCryptoUtilsTest.

- Adjust create and edit offer tests to model refactoring
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.

One minor comment. I think it's otherwise good.

To make it easier to review, it helps to keep refactorings separated in different commits. In particular pure refactor work and minor improvements are very helpful when kept separate.

ghubstan added a commit to ghubstan/bisq that referenced this pull request Oct 21, 2020
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

@sqrrm sqrrm merged commit b9e1398 into bisq-network:master Oct 21, 2020
@ghubstan ghubstan deleted the 1-convert-offerutil-to-injected-singleton branch October 22, 2020 15:18
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