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

Porting Manfred's HTTP API changes from bisq-core to the mono repo #1686

Closed
wants to merge 2 commits into from

Conversation

mrosseel
Copy link
Contributor

Manual port of the changes @ManfredKarrer did on his http-api branch in bisq-core.
Since the Bisq project switched to another repo structure in the mean time, I ported these changes over so they are quickly included in the master of Bisq and the API can make use of them.

@blabno
Copy link

blabno commented Sep 14, 2018

This is very important PR, blocking work on http-api.

@ManfredKarrer
Copy link
Contributor

Those changes are not well tested yet. I would prefer to keep them in the API branch and merge them later once the first feature set of the API is ready for merge.
You can just stay in sync with master and add those changes to your branch. I think that should not cause and blocking on your side, otherwise ping me on slack.

@ManfredKarrer
Copy link
Contributor

NACK

As discussed on Slack we prefer to keep those changes in the fork until we have it sufficiently tested.

@mrosseel
Copy link
Contributor Author

Hi Manfred, could you give some insight on what you need done to consider this PR as being sufficiently tested? The api needs some more refactoring to make it smaller and easier to review, so some kind of guideline on what to do to get these refactorings merged into master would be helpful!

@ManfredKarrer
Copy link
Contributor

They touch quite a few important areas (offer creation, tx fee estimation,...). When I have more time I will test it more carefully and for the next planned release mid October we will have a big test iteration anyway. So I would prefer to get it into that release but on a dev-test branch not on master to keep master low risk with potential bugs from not sufficiently tested code.

But why you need it in master? You can just fork Bisq master and add it there. We will keep that fork then as the API working base and once its time for getting a PR ready we will test and merge it into master.

I discussed a few weeks back with @blabno about a minimal API version what we can launch realistically. That should contain only the absolute min. set of features for a trader who wants to run a trading bot. I see those features limited to:

  • Offer creation
  • Take offer
  • Trade state changes (e.g. payment sent, etc)

I will not have time to review and test the whole API code soon, that would just take too much of my time and it is too critical and risky to get launched without very careful review and testing (not only for the API use but also to users who take an offer from an API user). We need to refactor also all the code duplication parts, I found several bugs there caused from not being in sync with master. That carries just too much risk and once it merged it likely will stay a technical debt and cause more problems in future. Most of those refactorings are rather trivial (moving code to core util classes or domain specific classes). Some are more complex (like the refactoring for fee estimation I did).

So the only way forward I see is to launch in iterations and keep the other features by default deactivated and if the user wants to activate it he gets a big warning that it is not tested and that he not only risks his funds but also can cause problems for other users (takers).
For the integration test all features can be activated, I am only referring to normal API users who should be limited to those areas which are reviewed and tested.

@blabno
Copy link

blabno commented Sep 18, 2018

@ManfredKarrer There is one very important reason why we want to get this in.
It contains startup refactoring, which we heavily depend on. If we don't merge it ASAP then it will diverge over time from master. Even if we keep merging master to that branch, we will hit conflicts sooner or later.

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.

3 participants