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.

Sign up for GitHub

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

Jump to bottom

Added the fix to make top 4 apis configurable #1673

Merged
merged 6 commits into from
Jun 11, 2019
Merged

Added the fix to make top 4 apis configurable #1673

merged 6 commits into from
Jun 11, 2019

Conversation

manikey123
Copy link
Contributor

@manikey123
Copy link
Contributor Author

@pmconrad @abitmore plz review the above pr for #783

abitmore
abitmore previously approved these changes Apr 23, 2019
View reviewed changes
@abitmore
Copy link
Member

OP tagged #783. Actually #783 was not for making the limitations configurable, but for adding limitations to some APIs, which has been done by @oxarbitrage in #1272 for 2.0.190219 release (but we didn't close that issue, nor mention the API changes in release notes (which is not good since it has broken some client applications)).

#782 was for changing the limitations to configurable, which is now closed, but we missed quite some APIs (mentioned in #783 (comment)).

IMHO we can merge this PR. However, I tend to close #783 and reopen #782 and add the missing APIs to the list in #782. @pmconrad @ryanRfox what's your opinion? maybe create a new issue?

pmconrad
pmconrad suggested changes Apr 24, 2019
View reviewed changes
tests/common/database_fixture.cpp Outdated Show resolved Hide resolved
tests/tests/database_api_tests.cpp Show resolved Hide resolved
tests/tests/database_api_tests.cpp Show resolved Hide resolved
tests/tests/database_api_tests.cpp Show resolved Hide resolved
@pmconrad
Copy link
Contributor

what's your opinion? maybe create a new issue?

IMO create a new issue. Modifying the old ones will only add to the existing confusion. Thanks for sorting this out!

@oxarbitrage
Copy link
Member

Pull currently having conflicts with merged htlc api limits. Will also conflict if #1749 gets merged before this one.

@manikey123
Copy link
Contributor Author

@pmconrad @abitmore plz review the updated PR

@pmconrad
Copy link
Contributor

pmconrad commented Jun 2, 2019

Changes look good.
Please rebase on latest develop to get rid of the conflicts.

@manikey123
Copy link
Contributor Author

@pmconrad the changes are updated and Travis has built successfully. Plz review

@pmconrad
Copy link
Contributor

pmconrad commented Jun 3, 2019

There are more duplicates now, please double-check.

@pmconrad pmconrad merged commit a2399ad into bitshares:develop Jun 11, 2019
@pmconrad pmconrad mentioned this pull request Jun 20, 2019
Closed
@abitmore abitmore mentioned this pull request Jun 22, 2019
Closed
14 tasks
@startailcoon startailcoon mentioned this pull request Jul 24, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmconrad pmconrad pmconrad approved these changes

@abitmore abitmore abitmore left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.2.0 - Feature Release
Development

Successfully merging this pull request may close these issues.
5 participants
@manikey123 @abitmore @pmconrad @oxarbitrage @ryanRfox