Details

Reviewers

deadalnix
Fabien

Group Reviewers

Restricted Project

Commits

rSTAGINGb2b842fed456: Remove most direct bitcoin calls from qt/walletmodel.cpp
rABCb2b842fed456: Remove most direct bitcoin calls from qt/walletmodel.cpp

Summary

Partial backport of Core PR 10244
https://github.com/bitcoin/bitcoin/pull/10244/commits/a0704a8996bb950ae3c4d5b5a30e9dfe34cde1d3
Progress towards T417
Depends on D2871, D2876

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

Branch

walletmodel

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 5596
Build 9254: Bitcoin ABC Buildbot (legacy)
Build 9253: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Apr 24 2019, 20:35

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 24 2019, 20:35

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

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

jasonbcox edited the summary of this revision. (Show Details)Apr 24 2019, 20:49

jasonbcox added a parent revision: D2876: [qt] Simplifies boolean expression model && model->haveWatchOnly().

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5593: Diff 8259.Apr 24 2019, 20:49

jasonbcox added inline comments.Apr 24 2019, 22:38

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

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

jasonbcox added inline comments.Apr 24 2019, 22:43

src/qt/transactionview.cpp
373	Depends on D2876, which is a separate stack of changes.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5596: Diff 8262.Apr 24 2019, 22:57

Fabien requested changes to this revision.Apr 25 2019, 08:06

Fabien added inline comments.

src/qt/sendcoinsdialog.cpp
16	This is removed in https://github.com/bitcoin/bitcoin/pull/10574
src/qt/transactionview.cpp
373	Could you rebase upon D2876 ?
src/qt/walletmodel.cpp
127	This is good, but seems unrelated ?
187	dito
227	Update the comments layout
280	dito
503	dito
527	dito

This revision now requires changes to proceed.Apr 25 2019, 08:06

jasonbcox added inline comments.Apr 25 2019, 16:02

src/qt/transactionview.cpp
373	Done.
src/qt/walletmodel.cpp
127	I went ahead and cleaned this up because it was tightly related to the chain I described in the below comment(s).
187	Required. See comment below.
227	I did this to match the original PR for reduced merge conflicts. It fits within our linting rules, so I left it.
280	Required. See comment below.
503	Required. See comment below.
527	This is subtle, but necessary. `wallet` -> `m_wallet` changed type in addition to naming. `m_wallet` doesn't have direct access to chainparams.

jasonbcox added inline comments.Apr 25 2019, 16:03

src/qt/sendcoinsdialog.cpp
16	This hasn't been backported yet but the looks of it?

Rebase

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5599: Diff 8271.Apr 25 2019, 16:19

Fabien requested changes to this revision.Apr 25 2019, 17:49

Fabien added inline comments.

src/qt/sendcoinsdialog.cpp
5	Please reorder for consistency.
16	This is a missing dependency.
src/qt/walletmodel.cpp
7	Dito
227	OK I let this up to you
527	Got it, thanks
src/qt/walletmodel.h
8	Please reorder.

This revision now requires changes to proceed.Apr 25 2019, 17:49

Fixed includes

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5603: Diff 8277.Apr 25 2019, 21:11

Fabien accepted this revision.Apr 26 2019, 11:43

This revision is now accepted and ready to land.Apr 26 2019, 11:43

Closed by commit rABCb2b842fed456: Remove most direct bitcoin calls from qt/walletmodel.cpp (authored by Russell Yanofsky <russ@yanofsky.org>, committed by jasonbcox). · Explain WhyApr 26 2019, 22:52

This revision was automatically updated to reflect the committed changes.

jasonbcox added a commit: rSTAGINGb2b842fed456: Remove most direct bitcoin calls from qt/walletmodel.cpp.Sep 18 2019, 23:09

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 8262

src/interfaces/node.h

src/interfaces/node.cpp

src/interfaces/wallet.h

src/interfaces/wallet.cpp

src/qt/bitcoin.cpp

src/qt/bitcoingui.cpp

src/qt/coincontroldialog.cpp

src/qt/overviewpage.cpp

src/qt/sendcoinsdialog.cpp

src/qt/signverifymessagedialog.cpp

src/qt/test/wallettests.cpp

src/qt/transactionview.cpp

src/qt/walletmodel.h

src/qt/walletmodel.cpp

src/qt/walletmodeltransaction.h

src/qt/walletmodeltransaction.cpp

src/qt/walletview.cpp

Remove most direct bitcoin calls from qt/walletmodel.cppClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 8262

src/interfaces/node.h

src/interfaces/node.cpp

src/interfaces/wallet.h

src/interfaces/wallet.cpp

src/qt/bitcoin.cpp

src/qt/bitcoingui.cpp

src/qt/coincontroldialog.cpp

src/qt/overviewpage.cpp

src/qt/sendcoinsdialog.cpp

src/qt/signverifymessagedialog.cpp

src/qt/test/wallettests.cpp

src/qt/transactionview.cpp

src/qt/walletmodel.h

src/qt/walletmodel.cpp

src/qt/walletmodeltransaction.h

src/qt/walletmodeltransaction.cpp

src/qt/walletview.cpp

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

Revision Contents
Changeset List