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

wallet.cpp file too big #950

Closed
2 of 9 tasks
oxarbitrage opened this issue May 22, 2018 · 11 comments
Closed
2 of 9 tasks

wallet.cpp file too big #950

oxarbitrage opened this issue May 22, 2018 · 11 comments
Assignees
Labels
2d Developing Status indicating currently designing and developing a solution 6 CLI Impact flag identifying the command line interface (CLI) wallet application code cleanup

Comments

@oxarbitrage
Copy link
Member

oxarbitrage commented May 22, 2018

https://github.com/bitshares/bitshares-core/blob/master/libraries/wallet/wallet.cpp

4500 lines and growing. this makes harder to edit the file with editors in older machines.

this issue is open to discuss what will be the best approach to split the file in several files or similar.

CORE TEAM TASK LIST

  • Evaluate / Prioritize Feature Request
  • Refine User Stories / Requirements
  • Define Test Cases
  • Design / Develop Solution
    • Assigned: @cogutvalera
    • Estimated: 15 hours
    • Remitted: Weeks 44-45
  • Perform QA/Testing
  • Update Documentation
@oxarbitrage oxarbitrage added code cleanup 2a Discussion Needed Prompt for team to discuss at next stand up. labels May 22, 2018
@abitmore abitmore added the 6 CLI Impact flag identifying the command line interface (CLI) wallet application label May 22, 2018
@cogutvalera
Copy link
Member

IMHO we should separate Wallet API into next different logical parts, perhaps like:

  1. graphene::wallet::commands_api
  2. graphene::wallet::account_api
  3. graphene::wallet::asset_api
  4. graphene::wallet::transaction_api
  5. graphene::wallet::history_api
  6. graphene::wallet::crypto_api
  7. graphene::wallet::network_api

Each logical part should be implemented in separated file.

@ryanRfox I want to claim this issue if possible. Thanks !

@oxarbitrage
Copy link
Member Author

The wallet.cpp file haves functions that are internal, member functions of the implementation and also wallet_api.

Separating wallet_api_impl and wallet_api in different files will reduce files size.

If we want to do categories i think they can be a bit different as the wallet is different than the api. Maybe a governance category can include witness, committee_member and worker stuff, wallet can include imports and such and so on. If we want to follow that path i can create a more detailed list of stuff that can be included on each category.

@cogutvalera
Copy link
Member

cogutvalera commented Oct 7, 2018

@oxarbitrage Thank you !

Separating wallet_api_impl and wallet_api in different files will reduce files size.

Currently wallet.cpp file has 4645 lines of code + comments (in develop branch).
So IMHO separating by 2 files isn't better than separating by different logical parts or categorize them as you suggest. My solution requires minor changes just to separate different logical parts in different files like for example database.cpp implementation.

@oxarbitrage your idea is really nice and requires more time & efforts to implement it fully & absolutely correctly ! IMHO your idea requires to create another new issue and new sub-issues maybe for such big changes.

Thanks !

@cogutvalera
Copy link
Member

@ryanRfox my estimation for this issue is approximately 15 hours. Thanks !

@ryanRfox
Copy link
Contributor

Assigned to @cogutvalera for 15 hours.

@ryanRfox ryanRfox added 2d Developing Status indicating currently designing and developing a solution and removed 2a Discussion Needed Prompt for team to discuss at next stand up. labels Oct 30, 2018
@cogutvalera
Copy link
Member

@ryanRfox Thanks !

@cogutvalera
Copy link
Member

PR is ready #1413

@cogutvalera
Copy link
Member

guys check please this PR #1413

@ryanRfox
Copy link
Contributor

ryanRfox commented Feb 3, 2019

Moving to (next) Feature Release because @cogutvalera has expressed interest in this and a few related Issues. My intent is the Core Team will discuss, prioritize and determine next steps by commenting within these Issues. We are going into a Release week, so our focus is elsewhere. Please expect an update near 13 FEB.

@cogutvalera
Copy link
Member

Thnaks !

@jmjatlanta
Copy link
Contributor

Fixed by PR #2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2d Developing Status indicating currently designing and developing a solution 6 CLI Impact flag identifying the command line interface (CLI) wallet application code cleanup
Projects
None yet
Development

No branches or pull requests

5 participants