Page MenuHomePhabricator

Remove most direct bitcoin calls from qt/walletmodel.cpp
ClosedPublic

Authored by jasonbcox on Apr 24 2019, 20:35.

Details

Summary
Test Plan

make check
ninja check
bitcoin-qt # sanity checks; perform transactions and make sure affected dialog boxes behave as expected

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Doing another round of self review and annotating deviations from the original PR.

src/interfaces/node.h
151 ↗(On Diff #8259)

getTxConfirmTarget was left out because confirm target is not supported

src/interfaces/wallet.cpp
19 ↗(On Diff #8259)
  • TxId is used instead of uint256
  • feebumper doesn't exist in ABC
src/interfaces/wallet.h
149 ↗(On Diff #8259)

Address type and change type do not exist in ABC.

src/qt/sendcoinsdialog.cpp
223 ↗(On Diff #8259)

nConfTarget no longer supported

src/qt/transactionview.cpp
416 ↗(On Diff #8259)

bumping txs (RBF) isn't supported by ABC

src/qt/walletmodel.cpp
256 ↗(On Diff #8259)

Removed parentheses even though the original PR didn't because the locks no longer exist and there's no reason to maintain limited scope here.

301 ↗(On Diff #8259)

Removed parentheses even though the original PR didn't because the locks no longer exist and there's no reason to maintain limited scope here.

src/qt/walletmodel.h
218 ↗(On Diff #8259)

ABC doesn't support bumping fees, bumping txs, or conf target.

src/qt/walletmodeltransaction.cpp
23 ↗(On Diff #8259)

Virtual size is for segwit, which ABC doesn't support

Fixed minor changes that shouldn't have deviated from the backport

src/qt/transactionview.cpp
373 ↗(On Diff #8262)

Depends on D2876, which is a separate stack of changes.

Fabien requested changes to this revision.Apr 25 2019, 08:06
Fabien added inline comments.
src/qt/sendcoinsdialog.cpp
16 ↗(On Diff #8262)
src/qt/transactionview.cpp
373 ↗(On Diff #8262)

Could you rebase upon D2876 ?

src/qt/walletmodel.cpp
127 ↗(On Diff #8262)

This is good, but seems unrelated ?

187 ↗(On Diff #8262)

dito

227 ↗(On Diff #8262)

Update the comments layout

280 ↗(On Diff #8262)

dito

503 ↗(On Diff #8262)

dito

527 ↗(On Diff #8262)

dito

This revision now requires changes to proceed.Apr 25 2019, 08:06
src/qt/transactionview.cpp
373 ↗(On Diff #8262)

Done.

src/qt/walletmodel.cpp
127 ↗(On Diff #8262)

I went ahead and cleaned this up because it was tightly related to the chain I described in the below comment(s).

187 ↗(On Diff #8262)

Required. See comment below.

227 ↗(On Diff #8262)

I did this to match the original PR for reduced merge conflicts. It fits within our linting rules, so I left it.

280 ↗(On Diff #8262)

Required. See comment below.

503 ↗(On Diff #8262)

Required. See comment below.

527 ↗(On Diff #8262)

This is subtle, but necessary. wallet -> m_wallet changed type in addition to naming. m_wallet doesn't have direct access to chainparams.

src/qt/sendcoinsdialog.cpp
16 ↗(On Diff #8262)

This hasn't been backported yet but the looks of it?

Fabien requested changes to this revision.Apr 25 2019, 17:49
Fabien added inline comments.
src/qt/sendcoinsdialog.cpp
5 ↗(On Diff #8262)

Please reorder for consistency.

16 ↗(On Diff #8262)

This is a missing dependency.

src/qt/walletmodel.cpp
7 ↗(On Diff #8262)

Dito

227 ↗(On Diff #8262)

OK I let this up to you

527 ↗(On Diff #8262)

Got it, thanks

src/qt/walletmodel.h
8 ↗(On Diff #8262)

Please reorder.

This revision now requires changes to proceed.Apr 25 2019, 17:49
This revision is now accepted and ready to land.Apr 26 2019, 11:43
This revision was automatically updated to reflect the committed changes.