-
Notifications
You must be signed in to change notification settings - Fork 220
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 scanned transaction handling for one-sided payments with callbacks #3794
feat!: add scanned transaction handling for one-sided payments with callbacks #3794
Conversation
ac26982
to
a6ecf90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I feel like something is missing here. The scanned output faux transactions cannot be validated like normal transactions because we do not know their kernels. Their status needs to be driven by the status of the outputs, which for imported transaction was done using the check_imported_transactions
task but I can't see anything similar for ScannedUnconfirmed
outputs.
base_layer/wallet/src/transaction_service/protocols/transaction_validation_protocol.rs
Outdated
Show resolved
Hide resolved
a6ecf90
to
3d1029e
Compare
- Added separate statuses for faux transactions to enable unique events and validation for scanned one-sided transactions. - Added validation for one-sided payments updating the associated faux transaction mined height and confirmations. - Added callbacks for faux transactions (unconfirmed and confirmed). - Updated the wallet FFI with the callbacks.
3d1029e
to
fb1a416
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. My one comment is just a Nit so approving and going start a merge queue
impl TransactionStatus { | ||
pub fn is_faux(&self) -> bool { | ||
match self { | ||
TransactionStatus::Completed => false, | ||
TransactionStatus::Broadcast => false, | ||
TransactionStatus::MinedUnconfirmed => false, | ||
TransactionStatus::Imported => true, | ||
TransactionStatus::Pending => false, | ||
TransactionStatus::Coinbase => false, | ||
TransactionStatus::MinedConfirmed => false, | ||
TransactionStatus::Rejected => false, | ||
TransactionStatus::FauxUnconfirmed => true, | ||
TransactionStatus::FauxConfirmed => true, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
impl TransactionStatus { | |
pub fn is_faux(&self) -> bool { | |
match self { | |
TransactionStatus::Completed => false, | |
TransactionStatus::Broadcast => false, | |
TransactionStatus::MinedUnconfirmed => false, | |
TransactionStatus::Imported => true, | |
TransactionStatus::Pending => false, | |
TransactionStatus::Coinbase => false, | |
TransactionStatus::MinedConfirmed => false, | |
TransactionStatus::Rejected => false, | |
TransactionStatus::FauxUnconfirmed => true, | |
TransactionStatus::FauxConfirmed => true, | |
} | |
} | |
impl TransactionStatus { | |
pub fn is_faux(&self) -> bool { | |
match self { | |
TransactionStatus::Imported | | |
TransactionStatus::FauxUnconfirmed | | |
TransactionStatus::FauxConfirmed => true, | |
_ => false, | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will fix that as a side issue when I submit a new PR based on this merge.
Description
Motivation and Context
Currently when a one-sided output is found by scanning the blockchain a Faux transaction is created for it with the Imported status, the same as for a manually imported output. Outputs found by scanning (for one-sided payments) should be handled differently to a manual import and their mined height and number confirmations should be validated.
How Has This Been Tested?