Skip to content
This repository was archived by the owner on Feb 15, 2024. It is now read-only.

Modularize wallet code#88

Open
aido wants to merge 2 commits intojgarzik:masterfrom
aido:wallet
Open

Modularize wallet code#88
aido wants to merge 2 commits intojgarzik:masterfrom
aido:wallet

Conversation

@aido
Copy link
Contributor

@aido aido commented Sep 4, 2016

Hi,

This pull request modularizes the wallet code, placing wallet code and AES code in their own library. This modularization is in line with the direction bitcoin-core is also going.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.363% when pulling 6d567c7 on aido:wallet into b630156 on jgarzik:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.363% when pulling 6d567c7 on aido:wallet into b630156 on jgarzik:master.

@coveralls
Copy link

coveralls commented Sep 4, 2016

Coverage Status

Coverage remained the same at 79.363% when pulling 72fbf49 on aido:wallet into b630156 on jgarzik:master.

@jgarzik
Copy link
Owner

jgarzik commented Sep 11, 2016

I think moving src/wallet.c to lib/wallet/wallet.c exposes some layering issues. It is unusual and avoidable to create a library routine -- wallet_filename() -- and then require the upper layer program to implement it.

src/wallet.c was written to be a module of the wallet program (picocoin), and moving this to lib/wallet is premature. It is better to move functionality by code movement to lib/wallet, and let lib/wallet grow organically as the picocoin wallet is built.

The other 25% or so of this patch is ok :)

@aido aido force-pushed the wallet branch 3 times, most recently from 1ab7ddf to 0c0af41 Compare September 13, 2016 20:28
@aido
Copy link
Contributor Author

aido commented Sep 13, 2016

Fair point.
Trying to pack ALL wallet related code into one place the way I did was a terrible idea not very well executed.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.363% when pulling 92d83c6 on aido:wallet into b630156 on jgarzik:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.363% when pulling 92d83c6 on aido:wallet into b630156 on jgarzik:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.363% when pulling 92d83c6 on aido:wallet into b630156 on jgarzik:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.363% when pulling 92d83c6 on aido:wallet into b630156 on jgarzik:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.363% when pulling 92d83c6 on aido:wallet into b630156 on jgarzik:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.363% when pulling 92d83c6 on aido:wallet into b630156 on jgarzik:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.363% when pulling 92d83c6 on aido:wallet into b630156 on jgarzik:master.

@aido
Copy link
Contributor Author

aido commented Dec 23, 2016

Hi,

The most recent commit to this PR tries to modularize the wallet code a small bit more. It organizes some of the wallet code into it's own subdirectory and also it's own separate library (noinst_LTLIBRARIES = libccoinwallet.la).. This library will expand as the wallet is built and grows.

@aido aido force-pushed the wallet branch 2 times, most recently from 41fc2ac to 7c53663 Compare December 23, 2016 16:18
@coveralls
Copy link

coveralls commented Dec 23, 2016

Coverage Status

Coverage remained the same at 76.852% when pulling 7c53663 on aido:wallet into 3c3d504 on jgarzik:master.

@libbitc
Copy link
Contributor

libbitc commented Feb 1, 2017

Hi @jgarzik,

I have added to libbitc the 25% :-) of this pull request that was OK and further built on it. Although this PR has since been tidied up a bit.

On top of this, libbitc no longer depends on libjansson and has instead embedded the ultralightweight JSON parser cJSON.

See:
libbitc@631224c

If you wish I can create another pull request to remove the libjansson dependency from picocoin also.

@coveralls
Copy link

coveralls commented Jun 29, 2017

Coverage Status

Coverage remained the same at 78.583% when pulling ae596e7 on aido:wallet into faa9b83 on jgarzik:master.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants