[GRPH-49] get_account_history fix #69
Conversation
tests/tests/account_history_test.cpp
Outdated
| * THE SOFTWARE. | ||
| */ | ||
| /* | ||
| * Copyright (c) 2019 PBSA, and contributors. |
There was a problem hiding this comment.
I think this should be placed on top in the line of Cryptonomex.
tests/tests/account_history_test.cpp
Outdated
| #include <fc/smart_ref_impl.hpp> | ||
| #include <fc/crypto/digest.hpp> | ||
|
|
||
|
|
There was a problem hiding this comment.
I think we can avoid double newlines.
tests/tests/account_history_test.cpp
Outdated
| auto dan_acc = create_account("dan"); | ||
| auto bob_acc = create_account("bob"); | ||
|
|
||
|
|
There was a problem hiding this comment.
I know this new lines are also in bitshares, from the code is coming from. I think we can remove them, also sent pull request at bitshares for it bitshares/bitshares-core#1955
There was a problem hiding this comment.
I am not totally sure of what we should do here but i am thinking on directly add a new patch that was added to the same function(get_account_history) in bitshares at bitshares/bitshares-core#628 on top of the one you added.
Also i think it might worth to add the additional history api tests from bitshares:
https://github.com/bitshares/bitshares-core/blob/master/tests/tests/history_api_tests.cpp#L86
This defines how the pagination for the call works(what results and what order you will get when calling with different stop and start arguments).
There was a problem hiding this comment.
@gladcow - please look into the changes requested by @oxarbitrage
|
I've addressed the issues found by @oxarbitrage , re-review, please. |
There was a problem hiding this comment.
- Please start Gitflow from from develop branch
- Send the PR to develop
reference: https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow
git checkout develop
git checkout -b feature/GRPH-49
git checkout develop
git merge feature/GRPH-49
or use git flow extension
|
I've redirected PR to |
| * Copyright (c) 2019 PBSA, and contributors. | ||
| */ | ||
| /* | ||
| * Copyright (c) 2015 Cryptonomex, Inc., and contributors. |
There was a problem hiding this comment.
I think this is still wrong. Just keep 1: Copyright (c) 2019 PBSA, and contributors. Put it in the line where Copyright (c) 2015 Cryptonomex, Inc., and contributors. is.
There was a problem hiding this comment.
In my opinion remove the Cryptonomex line fully and leave only the PBSA one :)
Backports Bitshares fix bitshares/bitshares-core#398 and adds unit tests for this fix.