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

[Draft] Shield Funds from cached UTXOs #339

Closed
wants to merge 1 commit into from

Conversation

pacu
Copy link
Contributor

@pacu pacu commented Feb 3, 2021

The Idea of this PR is start to improve this PoC to the point it can be merged into the rust Crates.

Background: During the Holiday break I had the idea of putting together a Proof of Concept of how we could manage to shield funds from a t-address a light client using the data-access-api branch (now on master) and adding the needed functionality sot that:

given a set of UTXOs for a t-address obtained from lightwalletd, a light client could store them in a cache (BlockDb) and invoke a function in librustzcash that would take those funds and shield them to a z-address derived from a spending key. The user would have to use the Transparent Secret Key corresponding to those transparent outputs as proof of spendability

Librustzcash should be able to get all the UTXOs from the shared cache and build a shielding transaction

remove unused imports

rename index column of utxos table

fix: move Script back to Vec<u8>

Add missing imports

[WIP] move code to chain.rs and catch up with latest changes

reset wallet.rs

fix SQL query
@pacu pacu requested review from nuttycom and str4d February 3, 2021 22:04
Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

I'm happy to pair with you to fix up any/all the issues so far! But I have a larger question, shouldn't the actual construction of the autoshielding transaction be done using the transaction builder in the zcash_client_backend trait? Right now you must be doing that outside of the rust code, right?


use crate::{error::SqliteClientError, BlockDB};

pub mod init;


pub struct UnspentTransactionOutput {
Copy link
Contributor

Choose a reason for hiding this comment

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

This type should probably go in zcash-client-backend; we don't actually want any functions depending on any types or calling anything in zcash_client_sqlite directly; the only things that should be used from this crate (not through the DAA) are the WalletDB and BlockDB initialization functions.

anchor_height: BlockHeight,
address: &str
) -> Result<Vec<UnspentTransactionOutput>,SqliteClientError> {
let mut stmt_blocks = cache.0.prepare(
Copy link
Contributor

Choose a reason for hiding this comment

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

What is inserting data into this table?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rust welding on mobile wallets. The idea is that libruszcash only reads from this table as it only reads the compact_blocks table. Should I add the insertion code as well to make this 'self-sufficient' ? what's the compact_blocks approach?

params: &P,
cache: &BlockDB,
anchor_height: BlockHeight,
address: &str
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this address be wrapped in a better type? You could pass the Magna Carta here. :)

. ok_or(format!(
"Could not interpret {} as a valid Zcash address.",
addr
)).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

unwrap() should never be used, this error needs to be propagated out. Here's an example of something similar: https://github.com/zcash/librustzcash/blob/master/zcash_client_sqlite/src/wallet.rs#L79-L85

txid: txid,
index: index,
script: script,
value: Amount::from_i64(value).unwrap(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about unwrap() here, you don't want to crash the process.

let mut utxos = Vec::<UnspentTransactionOutput>::new();

for utxo in rows {
utxos.push(utxo.unwrap())
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, no unwrap, this is a place to use collect()

@nuttycom nuttycom marked this pull request as draft February 3, 2021 22:22
@codecov
Copy link

codecov bot commented Feb 3, 2021

Codecov Report

Merging #339 (cdc94e5) into master (b5ee057) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #339   +/-   ##
=======================================
  Coverage   64.88%   64.88%           
=======================================
  Files          69       69           
  Lines        7066     7066           
=======================================
  Hits         4585     4585           
  Misses       2481     2481           
Impacted Files Coverage Δ
zcash_client_sqlite/src/chain.rs 97.44% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5ee057...dd2972d. Read the comment docs.

@nuttycom
Copy link
Contributor

Subsumed by #341

@nuttycom nuttycom closed this Feb 13, 2021
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.

2 participants