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

remove get_named_account_balances #1135

Closed
6 of 8 tasks
oxarbitrage opened this issue Jul 12, 2018 · 14 comments
Closed
6 of 8 tasks

remove get_named_account_balances #1135

oxarbitrage opened this issue Jul 12, 2018 · 14 comments
Labels
1c Task Task for team member to perform. Description may contain a Task List and reference child Sub-Tasks 2c Ready for Development Status indicating the Requirements are sufficent to begin designing a solution 4b Normal Priority Priority indicating the moderate impact to system/user -OR- existing workaround is costly to perform 6 API Impact flag identifying the application programing interface (API) good first issue

Comments

@oxarbitrage
Copy link
Member

oxarbitrage commented Jul 12, 2018

User Story

As a developer I think get_named_account_balances can be removed so the code is easier to maintain.

While making a research on the impact of #1051 i found that get_account_balances already covers what get_named_account_balances do, as the former already accepts an account string.
Update: this is caused by the merge of: #989

I understand there can be some legacy issues by removing get_named_account_balances but there is no point on having it either. I propose to remove it.

Code Reference: https://github.com/bitshares/bitshares-core/blob/master/libraries/app/include/graphene/app/database_api.hpp#L312-L321

Impacts
Describe which portion(s) of BitShares Core may be impacted by your request. Please tick at least one box.

  • API (the application programming interface)

CORE TEAM TASK LIST

  • Evaluate / Prioritize Feature Request
  • Refine User Stories / Requirements
  • Design / Develop Solution
    • Issue estimation: 0.5 hours
    • Issue assigned to @cogutvalera
  • Perform QA/Testing
  • Update Documentation
@oxarbitrage oxarbitrage added good first issue 1c Task Task for team member to perform. Description may contain a Task List and reference child Sub-Tasks 2a Discussion Needed Prompt for team to discuss at next stand up. 4b Normal Priority Priority indicating the moderate impact to system/user -OR- existing workaround is costly to perform 5a Docs Needed Status specific to Documentation indicating the need for proper documentation to be added 6 API Impact flag identifying the application programing interface (API) labels Jul 12, 2018
@abitmore
Copy link
Member

I recommend that we keep this API to avoid breaking existing client apps, and document it. We can change the code (the implementation of this API) to call the other API to reduce future maintenance work load.

@oxarbitrage
Copy link
Member Author

good, lets do that.

@oxarbitrage oxarbitrage added 2c Ready for Development Status indicating the Requirements are sufficent to begin designing a solution and removed 2a Discussion Needed Prompt for team to discuss at next stand up. 5a Docs Needed Status specific to Documentation indicating the need for proper documentation to be added labels Jul 15, 2018
@cogutvalera
Copy link
Member

@abitmore @ryanRfox @oxarbitrage I want to claim this issue, is it possible to assign this issue for me ? Thanks !

@oxarbitrage
Copy link
Member Author

yes

@cogutvalera
Copy link
Member

Thanks !

@abitmore
Copy link
Member

@oxarbitrage I think it's already implemented as described. Just need to remove code that was commented out. Correct?

@cogutvalera
Copy link
Member

@abitmore where did you see that it was already implemented ?

@cogutvalera
Copy link
Member

cogutvalera commented Jul 18, 2018

as I've been understood this issue, if I'm not wrong, we don't need to remove comments actually for this issue, we need to remove not required code instead, or isn't so and we just need to remove only comments ?

@oxarbitrage
Copy link
Member Author

@oxarbitrage
Copy link
Member Author

oops @abit is right https://github.com/bitshares/bitshares-core/blob/develop/libraries/app/database_api.cpp#L882-L890 done in develop.
just need to remove the commented code.

@cogutvalera
Copy link
Member

funny issue :-)

@cogutvalera
Copy link
Member

I will make PR for this issue very soon :-)

@cogutvalera
Copy link
Member

PR #1153 was wrong because was based on master branch instead of develop branch that's why I've closed this PR and opened one new PR #1154

@oxarbitrage
Copy link
Member Author

closed by #1154

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1c Task Task for team member to perform. Description may contain a Task List and reference child Sub-Tasks 2c Ready for Development Status indicating the Requirements are sufficent to begin designing a solution 4b Normal Priority Priority indicating the moderate impact to system/user -OR- existing workaround is costly to perform 6 API Impact flag identifying the application programing interface (API) good first issue
Projects
None yet
Development

No branches or pull requests

3 participants