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!: update status #6008

Merged

Conversation

SWvheerden
Copy link
Collaborator

Description

Changes statuses of coinbase transactions and one-sided transactions

Motivation and Context

Transactions now show as one-sided or coinbase, not Faux anymore. This is more inline of what they are and shows more information to the user.

How Has This Been Tested?

manual + unit tests

Breaking Changes

Wallet database changes, and requires recovery to keep existing database.

Copy link

github-actions bot commented Dec 4, 2023

Test Results (Integration tests)

  2 files  +  2  12 suites  +12   1h 38m 38s ⏱️ + 1h 38m 38s
32 tests +32  27 ✔️ +27  0 💤 ±0    5 +  5 
42 runs  +42  27 ✔️ +27  0 💤 ±0  15 +15 

For more details on these failures, see this check.

Results for commit 7898aff. ± Comparison against base commit dee1892.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Dec 4, 2023
Copy link

github-actions bot commented Dec 4, 2023

Test Results (CI)

1 260 tests   1 260 ✔️  10m 6s ⏱️
     39 suites         0 💤
       1 files           0

Results for commit 7898aff.

♻️ This comment has been updated with latest results.

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.

Hi, some comments:

  • The states a one-sided or coinbase scanned output goes through are exactly the same, and adding differentiation to statuses just adds complexity to the code.
  • I think we should rather add an is_scanned column to the completed transactions (and potentially the outputs table as well) and track it that way - values could be 0 - not_scanned, 1 - one_sided and 2 - coinbase. (The outputs table already differentiates between the output types, but does not indicate if it was scanned or not.)
  • When an output is imported or scanned, the is_scanned column can be populated based on the output features output type and associated script.

base_layer/common_types/src/transaction.rs Show resolved Hide resolved
base_layer/common_types/src/transaction.rs Show resolved Hide resolved
@SWvheerden
Copy link
Collaborator Author

I disagree with the is_scanned column. The current wallet design works with those statuses to indicate what it is, where it came from, and in what state it is. Adding a new separate enum to indicate the status add complexity.

@SWvheerden
Copy link
Collaborator Author

SWvheerden commented Dec 5, 2023

I have added a tech debt issue for addressing after this release: #6010

This is much more inline of what it should be. But its a much bigger refactor.

@hansieodendaal
Copy link
Contributor

Wallet database changes, and requires recovery to keep existing database.

This is not strictly true, as only the statuses of old scanned coinbase transactions will be reported incorrectly.

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.

ACK

(system-level tests performed)

@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Dec 6, 2023
@SWvheerden SWvheerden merged commit e19ce15 into tari-project:development Dec 6, 2023
13 of 14 checks passed
@SWvheerden SWvheerden deleted the sw_change-coinbase_status branch December 8, 2023 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants