fix(wallet): save complete burn proof in file#7726
fix(wallet): save complete burn proof in file#7726sdbondi wants to merge 2 commits intotari-project:developmentfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the handling of burn proofs within the wallet, moving from a manual file path input to an automated system that saves complete burn proofs to a designated shared directory. This change aims to improve interoperability with L2 wallets by providing a standardized and accessible location for burn claim data. The console wallet's user interface has been simplified as a result, and the underlying transaction service now emits events upon successful burn proof confirmation and persistence. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
f38a803 to
a1eb4ee
Compare
There was a problem hiding this comment.
Code Review
This pull request refactors the burn proof saving mechanism to use a shared directory, which is a solid improvement for interoperability with L2 wallets. The logic has been moved to a more appropriate place in the transaction lifecycle. The changes are mostly clean and correct. I have a few suggestions to improve robustness and code style.
| kernel: proof.kernel, | ||
| value, | ||
| }; | ||
|
|
There was a problem hiding this comment.
The code writes a file but doesn't ensure the directory exists. This could lead to an error if the directory hasn't been created yet. It's safer to create the directory before attempting to write the file.
if let Some(parent) = final_path.parent() {
fs::create_dir_all(parent).await?;
}
fs::write(&final_path, serde_json::to_vec_pretty(&complete_proof)?).await?;| TransactionValidationFailed(OperationId, u64), | ||
| TransactionBurnConfirmed { | ||
| output_hash: HashOutput, | ||
| commitment: Box<CompressedCommitment>, |
There was a problem hiding this comment.
CompressedCommitment is a small, 32-byte Copy type. Boxing it with Box<CompressedCommitment> is unnecessary and introduces a heap allocation. It's better to store it directly. This also allows restoring the Hash derive on the TransactionEvent enum, which was removed in this PR.
This would also simplify the event creation in fetch_claim_burn_merkle_proofs.rs to commitment: burn.burn_proof.commitment.
| commitment: Box<CompressedCommitment>, | |
| commitment: CompressedCommitment, |
| fs::create_dir_all(&burn_proofs_dir).await?; | ||
| let kernel_merkle_proof = proof | ||
| .kernel_merkle_proof | ||
| .ok_or_else(|| anyhow!("No kernel_merkle_proof"))?; | ||
| let encrypted_data = proof.encrypted_data.ok_or_else(|| anyhow!("No encrypted_data"))?; |
There was a problem hiding this comment.
These ok_or_else calls can be made more idiomatic by using the Context trait from anyhow. This improves readability. You'll need to add use anyhow::Context; to the file.
| fs::create_dir_all(&burn_proofs_dir).await?; | |
| let kernel_merkle_proof = proof | |
| .kernel_merkle_proof | |
| .ok_or_else(|| anyhow!("No kernel_merkle_proof"))?; | |
| let encrypted_data = proof.encrypted_data.ok_or_else(|| anyhow!("No encrypted_data"))?; | |
| let kernel_merkle_proof = proof.kernel_merkle_proof.context("No kernel_merkle_proof")?; | |
| let encrypted_data = proof.encrypted_data.context("No encrypted_data")?; | |
| let value = proof.value.context("No value")?; |
a1eb4ee to
20f18fd
Compare
Description
fix(wallet): save complete burn proof in file
Motivation and Context
Use a shared file location for burn proofs, this allows L2 wallets to pick up burns for claiming
How Has This Been Tested?
Manually
What process can a PR reviewer use to test or verify this change?
Breaking Changes