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

feat: add persistence of transaction cancellation reason to wallet db #3842

Merged

Conversation

philipr-za
Copy link
Contributor

@philipr-za philipr-za commented Feb 15, 2022

Description

This PR adds the persistence of the reason a transaction was rejected or cancelled to the db. This is done by expanding the existing cancelled field on a CompletedTransaction too not just be a boolean but an integer encoded nullable Enum which will encode the reason for the cancellation. By expanding the information contained by this field we no longer need the valid field which was used to indicate when a Coinbase Transaction was abandoned and we just make that a cancellation reason. A diesel migration is provided to handle this change is db schema.

Some specific areas changed to handle the update:

  1. The console wallet was updated to take this change into account when displaying transactions.
  2. The transaction validation protocol was updated to handle coinbases using this field rather than the valid field
  3. Transaction database methods were updated to take the change into account

In the FFI we do the following:

  1. Remove the completed_transaction_is_valid method for the valid field that is taken out.
  2. Added the completed_transaction_get_cancellation_reason method that returns whether the cancelled field is null as -1 or else the integer encoding of the TxCancellationReason enum.
/// Gets the reason a TariCompletedTransaction is cancelled, if it is indeed cancelled
///
/// ## Arguments
/// `tx` - The TariCompletedTransaction
/// `error_out` - Pointer to an int which will be modified to an error code should one occur, may not be null. Functions
/// as an out parameter.
///
/// ## Returns
/// `c_int` - Returns the reason for cancellation which corresponds to:
/// | Value | Interpretation    |
/// |---  |---                  |
/// |  -1 | Not Cancelled       |
/// |   0 | Unknown             |
/// |   1 | UserCancelled       |
/// |   2 | Timeout             |
/// |   3 | DoubleSpend         |
/// |   4 | Orphan              |
/// |   5 | TimeLocked          |
/// |   6 | InvalidTransaction  |
/// |   7 | AbandonedCoinbase   |
/// # Safety
/// None
int completed_transaction_get_cancellation_reason(struct TariCompletedTransaction *transaction, int *error_out);

How Has This Been Tested?

Tests are updated to take this change into account.

This PR adds the persistence of the reason a transaction was rejected or cancelled to the db. This is done by expanding the existing `cancelled` field on a CompletedTransaction too not just be a boolean but an integer encoded nullable Enum which will encode the reason for the cancellation. By expanding the information contained by this field we no longer need the `valid` field which was used to indicate when a Coinbase Transaction was abandoned and we just make that a cancellation reason. A diesel migration is provided to handle this change is db schema.

Some specific areas changed to handle the update:
1. The console wallet was updated to take this change into account when displaying transactions.
2. The transaction validation protocol was updated to handle coinbases using this field rather than the valid field
3. Transaction database methods were updated to take the change into account

In the FFI we do the following:
1. Remove the `completed_transaction_is_valid` method for the valid that is taken out.
2. Added the `completed_transaction_get_cancellation_reason` method that returns whether the cancelled field is null as -1 or else the integer encoding of the TxCancellationReason enum.

```C
/// Gets the reason a TariCompletedTransaction is cancelled, if it is indeed cancelled
///
/// ## Arguments
/// `tx` - The TariCompletedTransaction
/// `error_out` - Pointer to an int which will be modified to an error code should one occur, may not be null. Functions
/// as an out parameter.
///
/// ## Returns
/// `c_int` - Returns the reason for cancellation which corresponds to:
/// | Value | Interpretation    |
/// |---  |---                  |
/// |  -1 | Not Cancelled       |
/// |   0 | Unknown             |
/// |   1 | UserCancelled       |
/// |   2 | Timeout             |
/// |   3 | DoubleSpend         |
/// |   4 | Orphan              |
/// |   5 | TimeLocked          |
/// |   6 | InvalidTransaction  |
/// |   7 | AbandonedCoinbase   |
/// # Safety
/// None
int completed_transaction_get_cancellation_reason(struct TariCompletedTransaction *transaction, int *error_out);
```

Tests are updated to take this change into account.
The Cucumber FFI test for cancellation is updated to test the new method
Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

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

LGTM
I just have a NIT about the nested if's. But they are fine as is.

@@ -235,14 +239,12 @@ impl TransactionsTab {
format!("{}", local_time.format("%Y-%m-%d %H:%M:%S")),
Style::default().fg(text_color),
)));
let status = if (t.cancelled || !t.valid) && t.status == TransactionStatus::Coinbase {
let status = if matches!(t.cancelled, Some(TxCancellationReason::AbandonedCoinbase)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not do something akin to this:

let status_msg = if let Some(reason) = tx.cancelled {
                format!("Cancelled: {}", reason)
            } else {
                tx.status.to_string()
            };

@@ -396,9 +395,9 @@ impl TransactionsTab {
);
let excess = Span::styled(tx.excess_signature.as_str(), Style::default().fg(Color::White));
let confirmation_count = app_state.get_confirmations(&tx.tx_id);
let confirmations_msg = if tx.status == TransactionStatus::MinedConfirmed && !tx.cancelled {
let confirmations_msg = if tx.status == TransactionStatus::MinedConfirmed && tx.cancelled.is_none() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: a match here would read easier than a nested if.
Something like:

|match (tx.status, confirmation_count, tx.cancelled){
(TransactionStatus::MinedConfirmed, _,None) => ...
...
}

@@ -166,7 +166,6 @@ message TransactionInfo {
bytes excess_sig = 9;
google.protobuf.Timestamp timestamp = 10;
string message = 11;
bool valid = 12;
Copy link
Contributor

Choose a reason for hiding this comment

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

minor but maybe worth commenting this out instead to show that 12 is still reserved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, that is true. Def how it should be done in the future though I don't think its a major issue at the moment as this is just used by the cucumber at this point.

@aviator-app aviator-app bot added mq-failed and removed mq-failed labels Feb 16, 2022
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

LGTM

Just a comment for future discussion.

Comment on lines +348 to +357
pub enum TxCancellationReason {
Unknown, // 0
UserCancelled, // 1
Timeout, // 2
DoubleSpend, // 3
Orphan, // 4
TimeLocked, // 5
InvalidTransaction, // 6
AbandonedCoinbase, // 7
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion point for future reference:

  • It might be cool to provide a bit more information to the user here, like the value of the time out or identifying which UTXO is implicated in the double spend when the culprit is just one of many dust inputs.

@aviator-app aviator-app bot merged commit 31410cd into tari-project:development Feb 16, 2022
sdbondi added a commit to sdbondi/tari that referenced this pull request Feb 17, 2022
* development: (28 commits)
  fix(wallet): fix aggressive disconnects in wallet connectivity (tari-project#3807)
  chore: honor decimals in ERC20 (tari-project#3809)
  chore: add icons to applications (tari-project#3812)
  fix: daily test (tari-project#3815)
  ci: improvements to macos pkg (tari-project#3824)
  ci: cargo test speedups (tari-project#3843)
  feat: add persistence of transaction cancellation reason to wallet db (tari-project#3842)
  fix: update RFC links and README (tari-project#3675) (tari-project#3839)
  chore: change NodeIdentity debug (tari-project#3817)
  fix(dan): include state_root in node hash (tari-project#3836)
  feat(cli): resize terminal height (tari-project#3838)
  feat(validator-node): initial state sync implementation (partial) (tari-project#3826)
  feat: update console wallet tui (tari-project#3837)
  feat: resize base node terminal on startup (tari-project#3827)
  feat(wallet): add grpc method for setting base node (tari-project#3828)
  chore(deps): bump follow-redirects from 1.14.5 to 1.14.8 in /applications/tari_web_extension_example (tari-project#3832)
  chore(deps): bump follow-redirects from 1.14.7 to 1.14.8 in /applications/tari_collectibles/web-app (tari-project#3833)
  chore(deps): bump follow-redirects from 1.14.5 to 1.14.8 in /applications/tari_web_extension (tari-project#3834)
  chore(deps): bump follow-redirects from 1.14.7 to 1.14.8 in /applications/launchpad/gui-vue (tari-project#3831)
  chore(deps): bump follow-redirects from 1.14.4 to 1.14.8 in /integration_tests (tari-project#3829)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants