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

WIP: Kraken buy multiple assets #1

Closed
wants to merge 35 commits into from
Closed

Conversation

Sebastix
Copy link
Owner

No description provided.

@Sebastix Sebastix changed the title Kraken buy multiple assets WIP: Kraken buy multiple assets Nov 24, 2020
@@ -0,0 +1,17 @@
FROM php:7.4-cli-alpine
Copy link

@Jorijn Jorijn Nov 25, 2020

Choose a reason for hiding this comment

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

Tip: Maak hier een multistage Dockerfile van, je kan dan de productie laag van de image hergebruiken voor development.

Voorbeeld:

FROM php:7.4-cli-alpine AS production

productie dingen hier

FROM production AS development

hier development dingen toevoegen

Copy link
Owner Author

Choose a reason for hiding this comment

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

Handy! Ik zag het ook in Dockerfile in de app root voorbij komen, ben niet bekend met het multistage concept binnen Docker maar sounds logic :) Kan dit dan ook in de Dockerfile geplaatst die in de app root staat?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ik heb nu een development stage toegevoegd in de Dockerfile:

##################################################################################################################
# Development Stage
##################################################################################################################
FROM production AS dev

RUN composer install

Copy link

Choose a reason for hiding this comment

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

Zeker! Je zal dan alleen tijdens het bouwen van bijv. de productiebuild moeten aangeven dat je tot een specifiek target wil gaan en niet verder.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Heb nu een development stage (helemaal onderaan) toegevoegd FROM base_image AS development in de Dockerfile in de app root. Hier staan verder (nog) geen commands in.
In docker/development/docker-compose.yml heb ik het volgende nu staan mbt de build:

    build:
      context: ../../
      dockerfile: Dockerfile
      target: development

Als ik echter daarna docker-compose --build doe, zou ik verwachten dat er een development image wordt gebouwd voor de service, zonder de test en production_build stages. Toch lijkt dit niet te gebeuren, want ik zie bij het builden de output van test voorbij komen.

Hetzelfde als ik docker build --target development -t jorijn/bitcoin-dca:development . uitvoer.

src/Command/BuyCommand.php Outdated Show resolved Hide resolved
src/Command/BuyCommand.php Outdated Show resolved Hide resolved
src/Command/BuyCommand.php Outdated Show resolved Hide resolved
src/Service/BuyService.php Outdated Show resolved Hide resolved
* @return CompletedBuyOrder
* @throws PendingBuyOrderException
*/
public function initiateBuy(int $amount, string $asset, bool $simulate = false): CompletedBuyOrder
Copy link

Choose a reason for hiding this comment

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

Je wijkt hier af van de interface wat in de regel niet zo'n goed idee is, buiten development is er eigenlijk ook geen reden om de aankoop te simuleren.

Copy link
Owner Author

Choose a reason for hiding this comment

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

De $simulate moet er uit inderdaad. Icm Docker die ziet of je in de development stage zit, zouden we dit wel kunnen gebruiken?

Copy link

Choose a reason for hiding this comment

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

Als je er op staat om perse een simulate optie er in te houden zou ik een extra service introduceren, deze kan dan de KrakenBuyService extenden of decoraten.

Door middel van prioriteiten aan je tags mee te geven kun je dan je KrakenSimulatedBuyService net boven de andere KrakenBuyService zetten. In de supports methode kun je dan controleren op de aanwezigheid van bijvoorbeeld KRAKEN_SIMULATE_BUY environment variabele.

Dit zou dan ongeveer zo uitwerken:

$ KRAKEN_SIMULATE_BUY=1 bin/bitcoin-dca buy 10 --yes

@@ -258,7 +274,7 @@ public function testGetRecipientAddress(): void
$this->dispatcher,
$this->logger,
$this->configuredExchange
))->getRecipientAddress();
))->getRecipientAddress($asset);
Copy link
Owner Author

Choose a reason for hiding this comment

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

@Jorijn Het zijn ondertussen aardig wat changes...teveel eigenlijk nu.
Elke WithdrawAddressProvider heb ik voorzien van een van een $asset property, zodat er gecheckt kan worden welk adres hoort bij welke asset (als je een andere suggestie hebt om dit op te lossen hoor ik dat graag).
Echter loop ik vast op 1 ding en dat is hier in de WithdrawServiceTest.php. Ik heb geen ervaring met het schrijven van PHP unit tests, dus ik begrijp niet goed wat ik hier moet oplossen:

 ✘ Get recipient address
   ┐
   ├ Expectation failed for method name is "provide" when invoked 2 time(s).
   ├ Method was expected to be called 2 times, actually called 0 times.     
   ┴

@Sebastix Sebastix closed this Mar 14, 2021
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.

None yet

2 participants