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

[GUI][Bug] Notify transaction creation failure reason #1626

Merged

Conversation

random-zebra
Copy link

Triggered by #1619

  • first commit fixes a bug where the WalletModel::message signal was not connected to pwidget, therefore the message with strFailReason -fired by prepareTransaction:

    PIVX/src/qt/walletmodel.cpp

    Lines 526 to 533 in f295ee4

    if (!fCreated) {
    if ((total + nFeeRequired) > nBalance) {
    return SendCoinsReturn(AmountWithFeeExceedsBalance);
    }
    Q_EMIT message(tr("Send Coins"), QString::fromStdString(strFailReason),
    CClientUIInterface::MSG_ERROR);
    return TransactionCreationFailed;
    }

    was never displayed, and the user was presented only with the generic "Transaction creation failed!" -fired by ProcessSendCoinsReturnAndInform:
    void ProcessSendCoinsReturnAndInform(PWidget* parent, const WalletModel::SendCoinsReturn& sendCoinsReturn, WalletModel* walletModel, const QString& msgArg, bool fPrepare)
    {
    CClientUIInterface::MessageBoxFlags informType;
    QString informMsg = ProcessSendCoinsReturn(parent, sendCoinsReturn, walletModel, informType, msgArg, fPrepare);
    if (!informMsg.isEmpty()) parent->emitMessage(parent->translate("Send Coins"), informMsg, informType, 0);
    }

  • second commit addresses the fact that, with the previous commit, the user is now presented with two consecutive dialogs: "Transaction too big" and "Transaction creation failed!", and merges them into one single message (which is already followed by a SnackBar with "Cannot create transaction").

  • third commit adds a clearer message when strFailReason is "Transaction too big" (which might be frequent e.g. sweeping a big number of masternode rewards or dust outputs). Reviewers are encouraged to offer better wording.

Closes #1619

@random-zebra random-zebra added this to the 5.0.0 milestone May 13, 2020
@random-zebra random-zebra self-assigned this May 13, 2020
@random-zebra random-zebra changed the title 202005 gui tx creation failure notif [GUI][Bug] Notify transaction creation failure reason May 13, 2020
@random-zebra random-zebra force-pushed the 202005_GUI_tx-creation-failure-notif branch from dffe3b1 to 05981ea Compare May 13, 2020 14:51
Copy link

@ambassador000 ambassador000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality tested, working as intended.

Just minor suggestion: coin control -> Coin Control to keep consistency.
txTooBig1

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK and code looking good, good fix.

Left a comment for the future.

src/qt/walletmodel.cpp Show resolved Hide resolved
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 05981ea

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 05981ea

@furszy furszy merged commit 5b064a6 into PIVX-Project:master May 23, 2020
@random-zebra random-zebra deleted the 202005_GUI_tx-creation-failure-notif branch September 24, 2020 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] Better feedback to the users when Transaction creation failed error appears
4 participants