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 renameCounterpartyToIssuer and formatBidsAndAsks #969

Closed
wants to merge 2 commits into from

Conversation

intelliot
Copy link
Collaborator

- Use formatBidsAndAsks with request instead of getOrderbook
- Deprecate getOrderbook, which does not sort orders correctly
- Resolve #766
@tuloski
Copy link
Collaborator

tuloski commented Nov 20, 2018

I'm confused on why this solves the other issue :).

Copy link
Collaborator Author

@intelliot intelliot left a comment

Choose a reason for hiding this comment

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

It deprecates getOrderbook and leaves its existing behavior unchanged. Some folks were concerned about changing the behavior of an existing method, even if it is broken -- someone might be relying on the broken behavior.

{ currency: 'BTC', issuer: 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B' }
```

## formatBidsAndAsks
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use this method instead of getOrderbook

@antondalgren
Copy link

I don't believe that deprecating a method that has the wrong behavior is the right way to go. I mean, this is a bug on the method and should be fixed and not replaced by yet another method. Also, getOrderbook is a lot more descriptive name than formatBidsAndAsks. I would still argue that fixing the method getOrderbook is a better solution.

How many times will a bug/faulty behavior deprecate a method and create a new one?

@sublimator
Copy link
Contributor

sublimator commented Nov 21, 2018 via email

@intelliot
Copy link
Collaborator Author

intelliot commented Nov 21, 2018

I mean, this is a bug on the method and should be fixed and not replaced by yet another method.

The concern is that some users may be relying on buggy behavior, so changing the method is technically a breaking change.

Also, getOrderbook is a lot more descriptive name than formatBidsAndAsks.

The methods do different things. With formatBidsAndAsks, you have to get the offers yourself (using request('book_offers', ...). You can then pass the offers to formatBidsAndAsks for nicer formatting.

How many times will a bug/faulty behavior deprecate a method and create a new one?

Every time a behavioral bug is found in a method that has been included in a stable release (e.g. 1.x.x non-beta), the non-breaking way to fix it is to add a new method and deprecate the buggy one. That's the only way a library can guarantee a stable API between minor/patch releases. Of course, if we were going to 2.0.0, then we could simply fix the method's behavior.

When we do advance to 2.0.0, deprecated methods will be deleted.

That said, I can certainly see the case for just applying the breaking change, as long as we're reasonably certain that it's not going to adversely affect anyone (i.e. nobody is relying on the existing behavior). I'll open a new PR for that. See #970

@tuloski
Copy link
Collaborator

tuloski commented Nov 22, 2018

If I had to maintain this I'd have created a new API directly. Like 2.0 leaving the old rippleAPI as it was, and starting to work on the new one, becase I see a lots of differences. The new API is more directly related to rippled API (lower level), while the old one was intended to be a higher level API.

@inmyth
Copy link

inmyth commented Nov 22, 2018

Actually I don't use ripple-lib much so I'm not invested in it as mush as other guys. Throughout my project I used ripple-lib-java. I did open the issue, a year and a half ago. I thought it was not maintained so I moved on (beside I don't really like Node/Js). Only recently all open issues are being addressed. Now looking at the solution, I think that's overkill for just fixing some sorting. Unless of course you want to move this lib to different direction. If this lib is to be turned into a set of helper tools then it's the way. I think I probably suggested this. Users should just feed raw response to some method to get human readable format. However you should talk with your team and other users about the features. Frankly the only I thing I need is a parser that interprets all node tags and offline signing. Sorting etc, I can handle myself.

@intelliot
Copy link
Collaborator Author

@inmyth Thanks. I started working on this project after you opened issue #766. This project is now being maintained, so it's worth going back and making sure old issues are addressed.

Yes, we do want to move this library in a different direction. See #812 for details. We have discussed this and we agree that this is the way to go :)

Copy link
Collaborator

@tuloski tuloski left a comment

Choose a reason for hiding this comment

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

If the goal is to remove all the "high level" request and move everything to "low level" request then here we are.
But I don't see why the lib cannot provide at the same time high level request as:

  • getBooks
  • autobridged books
  • Manage subscriptions and maintain books
  • Manage subscriptions and filter for example only payment transactions...
  • Signing transactions
  • ...

Because sincerely making everything a request is not much more than opening a ws and sending the raw request as described in the rippled API. The big problems the user have is in parsing the data received.

@intelliot
Copy link
Collaborator Author

Closing in favor of #970

@intelliot intelliot closed this Nov 26, 2018
@intelliot intelliot deleted the formatBidsAndAsks branch November 26, 2018 23:25
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.

5 participants