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

Fix #782: Change hard-coded limitations in API's to configurable #1513

Merged
merged 26 commits into from Mar 20, 2019

Conversation

Projects
5 participants
@manikey123
Copy link
Contributor

commented Jan 4, 2019

Added the review comments in request #1478 and test case for the api validation

This is wrt issue #782
Part A (Completed): get_account_history_operations -made FC_ASSERT limit configurable
and wrote the testcase for the same

Testcase Scenarios (Completed for PartA) :

  1. created 1 account with higher return limits(200)
  2. created 2 account with higher return limits(200)
  3. No asset_create op larger than id1 with higher return limits(200)
  4. Limited 1 returns 1 result
  5. alice has 1 op returns 1 result
  6. create a bunch of accounts and return set the limit to 125 and the max operation size to higher return limits(125)-- operation is limited by the max-operation-limit tickets:
    #1490

Part B: get_account_history - Do you want me to proceed with Part2 for get_account_history to make its FC_ASSSERT configurable?
Test cases already present

  1. get_account_history

Part C: Below apis have FC_ASSERT should they be made configurable?
Currently test cases not present. If they need to be implemented would need to know the scenarios

  1. get_grouped_limit_orders
  2. get_asset_holders_count
  3. get_asset_holders
  4. get_account_history_by_operations
  5. get_relative_account_history
  6. get_fill_order_history

manikey123 added some commits Dec 20, 2018

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

Feel free to continue with B and C.
IMO writing a full-blown test case for the other API calls is beyond the scope of #782. Would be good to check that the configured limit value works in each case though (in the sense that calling the API with a value below the configured limit returns something, possibly an empty result, and that calling it with a value above the limit throws as expected).

@jmjatlanta jmjatlanta referenced this pull request Jan 7, 2019

Closed

get_key_references performance issue? #1472

2 of 19 tasks complete

manikey123 added some commits Jan 8, 2019

Update history_api_tests.cpp
updated based on comments
Update history_api_tests.cpp
update the file for merge issue
Update application.cpp
updated int value
@manikey123

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

Can part A which I completed be merged with the master code
For parts B and C I can make a new PR. ?
@pmconrad @jmjatlanta

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

Code-wise I'd say this can be merged on its own, however, I would like to see a consistent naming scheme for the option names here and in the future PRs. For example, api-limit-<name_of_api_call>. IMO the general purpose of this option becomes clear immediately if you read only the first one or two components of the name.

manikey123 added some commits Jan 13, 2019

@manikey123

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2019

Added all the remaining parts for the below apis. Plz review
get_account_history
get_account_history_operations
get_grouped_limit_orders
get_relative_account_history
get_account_history_by_operations
get_asset_holders

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

Hint: use rebase -i and a force-push to get rid of the accidentally added file.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

One testcase is failing in travis. Can't immediately see what's wrong, please check and fix.

manikey123 added some commits Jan 15, 2019

Merge pull request #3 from bitshares/develop
Adding all the changes from the develop branch

@manikey123 manikey123 force-pushed the manikey123:782_test_p1 branch 2 times, most recently from 05d13ed to a324a18 Feb 24, 2019

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

Argh, you've destroyed the commit history!
Please check if you still have 906beb3 in your repo, then base the whitespace changes on top of that.

@manikey123 manikey123 force-pushed the manikey123:782_test_p1 branch 3 times, most recently from 74406e9 to 0d3a779 Feb 25, 2019

@manikey123

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2019

Please check if you still have 906beb3 in your repo, then base the whitespace changes on top of that.
whatever commit history I could find from my local repo I have commit that. and then committed the required changes over it plz do review

@pmconrad pmconrad moved this from In development to In testing in Feature Release (3.1.0) Mar 5, 2019

asset_api::~asset_api() { }

vector<account_asset_balance> asset_api::get_asset_holders( asset_id_type asset_id, uint32_t start, uint32_t limit ) const {

vector<account_asset_balance> asset_api::get_asset_holders( std::string asset, uint32_t start, uint32_t limit ) const {

This comment has been minimized.

Copy link
@pmconrad

pmconrad Mar 6, 2019

Contributor

why?

This comment has been minimized.

Copy link
@abitmore

abitmore Mar 6, 2019

Member

This change is already in develop branch. #1272

This comment has been minimized.

Copy link
@manikey123

manikey123 Mar 7, 2019

Author Contributor

it was added after I made the updates for the PR. had to merge the changes for the develop branch

This comment has been minimized.

Copy link
@pmconrad

pmconrad Mar 7, 2019

Contributor

This should be handled during the merge, not in advance.

This comment has been minimized.

Copy link
@abitmore

abitmore Mar 8, 2019

Member

@pmconrad IIRC there was a conflict, fixed by merging develop branch into this branch. IMHO it's the correct approach. Actually, if you check the whole PR diff or the "condensed" merge commit but not the full merge commit, you won't see this change.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Mar 10, 2019

Contributor

Merge conflicts should be fixed during the merge.

In this case, some code was c&p'ed from develop, maybe in order to avoid a merge conflict. That's the wrong approach.

I know the accidentally added / removed code doesn't show up in the full PR diff, but I'd like to have a clean and lean history. That's why git rebase -i exists.

@pmconrad
Copy link
Contributor

left a comment

Thanks for restoring history. c0a1109 is what I originally approved I believe.

I'm confused by your later commits though. Why do you add tests only to remove them again? If that was an accident, please squash the two commits into one and force-push.

There's an unrelated API change in there, probably also by accident.

@manikey123 manikey123 force-pushed the manikey123:782_test_p1 branch from adc0086 to a4108e0 Mar 17, 2019

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

Thanks. Squashed one too many but that doesn't hurt. ba3a895 is the parent of the abovementioned c0a1109, and a4108e0 include the changes from that.
Now please merge develop and resolve the conflicts.

manikey123 added some commits Mar 18, 2019

Update asset_api_tests.cpp
updated for test case to match the input type of the api after merge with develop branch
@pmconrad
Copy link
Contributor

left a comment

Looks good now. Thanks for your patience. :-)

@abitmore ?

@abitmore

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

Personally I don't like some coding styles appeared in the code: tabs, long lines (>118 characters), inconsistent indentations/padding/spacing E.G. sometimes no white spaces around = or after ,, trailing white spaces(old code), and etc. The code itself looks fine (I didn't thoroughly check).

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

To be fair, the diff shows some very long lines in the unchanged parts as well.
I expect the complete codebase will be reformatted in the context of #1318 anyway, so I don't think it will pay off now.

@pmconrad pmconrad merged commit ae78c2d into bitshares:develop Mar 20, 2019

2 checks passed

ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Feature Release (3.1.0) automation moved this from In testing to Done Mar 20, 2019

@pmconrad pmconrad referenced this pull request Apr 29, 2019

Closed

add htlc api calls #1729

@pmconrad pmconrad removed this from In Testing in Community Claims Apr 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.