Page MenuHomePhabricator

Convert wallet and qt modules to use Amount struct (T104)
AbandonedPublic

Authored by schancel on Oct 14 2017, 18:04.

Details

Reviewers
freetrader
deadalnix
Group Reviewers
Restricted Project
Summary

Converts qt and wallet modules to use Amount struct together.
These two modules are fairly intertwined, so converting them
together avoids a significant amount of intermediate patches.

Test Plan

make VERBOSE=1 check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
shammah/amount-wallet-and-qt
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 1057
Build 1057: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Oct 15 2017, 18:48
deadalnix added inline comments.
src/qt/bitcoinamountfield.h
23 ↗(On Diff #1555)

Worth looking at what that discussion says and update the comment accordingly.

Why did you start using qint64 instead of Amount here ? Is the discussion still relevant ?

src/qt/bitcoingui.cpp
1028 ↗(On Diff #1555)

Remove reference

src/qt/bitcoingui.h
201 ↗(On Diff #1555)

Remove reference.

src/qt/bitcoinunits.cpp
94 ↗(On Diff #1555)

Use C++ style constructor

qint64(nIn.GetSatoshis());
src/qt/bitcoinunits.h
80 ↗(On Diff #1555)

Remove reference

84 ↗(On Diff #1555)

Remove reference

88 ↗(On Diff #1555)

Remove reference

91 ↗(On Diff #1555)

Same question about the qint64

118 ↗(On Diff #1555)

Maybe that should be a static method on Amount instead ?

src/qt/coincontroldialog.cpp
455 ↗(On Diff #1555)

Remove reference

src/qt/guiutil.h
56 ↗(On Diff #1555)

Remove reference

src/qt/overviewpage.h
45 ↗(On Diff #1555)

Remove reference x6

src/qt/paymentserver.cpp
565 ↗(On Diff #1555)

This comment is not accurate anymore.

src/qt/paymentserver.h
92 ↗(On Diff #1555)

Remove reference

src/qt/sendcoinsdialog.h
62 ↗(On Diff #1555)

Remove reference x6

src/qt/transactionfilterproxy.h
41 ↗(On Diff #1555)

Remove reference

src/qt/transactionrecord.cpp
135 ↗(On Diff #1555)

Maybe implementing the unary minus op would be worth it ?

src/qt/transactionrecord.h
111 ↗(On Diff #1555)

Remove reference x2

src/qt/walletmodel.h
43 ↗(On Diff #1555)

Remove reference

268 ↗(On Diff #1555)

Remove reference x6

src/qt/walletmodeltransaction.h
30 ↗(On Diff #1555)

Remove reference

src/qt/walletview.h
133 ↗(On Diff #1555)

Remove reference

src/validation.h
386 ↗(On Diff #1555)

This should probably not be boxed in that diff, please split it. Consensus code change should not be bundled with all the QT changes.

src/wallet/test/wallet_tests.cpp
40 ↗(On Diff #1555)

Remove reference

This revision now requires changes to proceed.Oct 15 2017, 18:48
schancel edited edge metadata.

Rebase on top of D609

I have a few more changes to make after the latest diff, not quite ready for review yet.

src/qt/bitcoinamountfield.h
23 ↗(On Diff #1555)

I wanted to ask someone more familiar with QT. The moc files that are generated when using Amount do not work properly. I haven't been able to figure out how to fix them. The comment is still applicable. It's because the auto-generated code does not include amount.h.

src/qt/bitcoinunits.cpp
94 ↗(On Diff #1555)

Sounds good to me.

Just to clarify: We're okay with including stylistic changes in patches? Normally I've been required to do stylistic changes as part of a larger patch to change things everywhere at once.

src/qt/bitcoinunits.h
118 ↗(On Diff #1555)

Yeah, this was a bit weird. There's already a max money around. I can move it.

src/qt/transactionrecord.cpp
135 ↗(On Diff #1555)

I think there's only about 3 uses in the entire codebase. I also tend to think that negating money shouldn't be a convenient operation.