-
Notifications
You must be signed in to change notification settings - Fork 137
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
Stellar Protocol 11 Compatibility #186
Conversation
@bartekn @morleyzhi this is up for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fnando My only suggestion here is to update the changelog right now, if only to signal to whoever releases this that a breaking change was made. (Since it'll probably be me, I'll remember, but just in case I don't)
@morleyzhi good call. Totally forgot about the changelog. Just pushed the update. |
- Add Stellar Protocol 11 compatibility (breaking change) | ||
- Rename `manage_offer` to `manage_sell_offer`. | ||
- Rename `create_passive_offer` to `create_passive_sell_offer`. | ||
- Add `manage_buy_offer`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some things I noticed:
- The breaking change warning isn't super visible. The changelog should have earlier updates that bold the words and put it at the beginning, I think it's worth maintaining that convention
- If I'm just a JS dev, I might not understand what implications this has to the API of the library, so it might make sense to frame this in terms of the library exports that are being added (I guess it's renaming
Operation.manageOffer
toOperation.manageSellOffer
?)
Close #179
manage_offer
tomanage_sell_offer
create_passive_offer
tocreate_passive_sell_offer
manage_buy_offer
(CAP-0006)