Problem:(CRO-557) no way to import transactions (without a view key) #695

linfeng-crypto · 2019-12-17T10:15:42Z

solution:

add a interface export_tx for sender to get the plain transaction
add a interface import_tx for receiver to import the plain transaction and get the output UTXO

codecov · 2019-12-17T10:55:11Z

Codecov Report

Merging #695 into master will decrease coverage by 0.55%.
The diff coverage is 8.21%.

@@            Coverage Diff             @@
##           master     #695      +/-   ##
==========================================
- Coverage   69.71%   69.16%   -0.56%     
==========================================
  Files         131      131              
  Lines       16745    16886     +141     
==========================================
+ Hits        11674    11679       +5     
- Misses       5071     5207     +136

Impacted Files	Coverage Δ
client-common/src/lib.rs	`100% <ø> (ø)`	⬆️
chain-core/src/tx/data/input.rs	`23.52% <0%> (-3.14%)`	⬇️
client-core/src/wallet/default_wallet_client.rs	`42.73% <0%> (-11.62%)`	⬇️
client-common/src/transaction.rs	`50% <0%> (-4.06%)`	⬇️
...builder/unauthorized_wallet_transaction_builder.rs	`0% <0%> (ø)`	⬆️
client-core/src/service/wallet_service.rs	`85.11% <0%> (-3.16%)`	⬇️
...tion_builder/default_wallet_transaction_builder.rs	`88.58% <0%> (-2.93%)`	⬇️
client-cli/src/command/transaction_command.rs	`0% <0%> (ø)`	⬆️
client-rpc/src/rpc/wallet_rpc.rs	`63.24% <0%> (-1.48%)`	⬇️
client-core/src/wallet/syncer.rs	`60.77% <100%> (+0.56%)`	⬆️
... and 3 more

tomtau

looks good (I haven't tested it yet though), just a bit more descriptive comments would be helpful + as this is a bigger feature, it'll be good to update CHANGELOG.md that this feature was added (just put a note at the top with a header ## v0.1.1 (unreleased) )

tomtau · 2019-12-18T01:36:42Z

client-core/src/wallet.rs

@@ -185,6 +185,9 @@ pub trait WalletClient: Send + Sync {

    /// Broadcasts a transaction to Crypto.com Chain
    fn broadcast_transaction(&self, tx_aux: &TxAux) -> Result<BroadcastTxResponse>;
+
+    /// Get the plain transaction


this comment just repeats the name, so not that helpful -- it'd be better to include some information that one can't read from the type signature... e.g. under what circumstances it could fail or what kind of String it returns (it seems to return base64-encoded ?)

tomtau · 2019-12-18T01:38:08Z

client-common/src/transaction.rs

+pub struct TransactionInfo {
+    /// enum Transaction type
+    pub tx: Transaction,
+    /// block height of the tx


I guess block height when the transaction was committed?

tomtau · 2019-12-18T01:42:03Z

client-cli/src/command.rs

+        about = "Download a plain transaction by a given transaction id"
+    )]
+    DownloadTx {
+        #[structopt(name = "name", short, long, help = "Name of wallet")]
+        name: String,
+        #[structopt(name = "tx_id", short, long, help = "transaction id")]
+        tx_id: String,
+    },
+
+    #[structopt(
+        name = "sync_tx",
+        about = "Synchronize a plain transaction that view key not included"


in about, it may good to list the transaction types this applies to

also how about
./client-cli transaction download --name a
sub-command of transaction

download_tx is too long to type

yes, I think so.

leejw51crypto · 2019-12-18T02:24:31Z

overall, lgtm,
could you add some unit-test?

yihuang · 2019-12-18T12:08:13Z

client-core/src/transaction_builder/unauthorized_wallet_transaction_builder.rs

@@ -29,4 +30,8 @@ impl WalletTransactionBuilder for UnauthorizedWalletTransactionBuilder {
    fn obfuscate(&self, _: SignedTransaction) -> Result<TxAux> {
        Err(ErrorKind::PermissionDenied.into())
    }
+
+    fn decrypt_tx(&self, _txid: TxId, _private_key: &PrivateKey) -> Result<Transaction> {


The only two methods of trait TransactionObfuscation get re-exported by trait WalletTransactionBuilder, it doesn't feel right ;D

I think if I don't use the WalletTransactionBuilder, I have to put a TransactionObfuscation implied object into the DefaultWalletClient and I have to deal with the new_read_only function where I have to create a new mock transaction obfuscation like UnauthorizedObfuscation and make impl the TransactionObfuscation, which is much expensive.

yihuang · 2019-12-18T12:12:50Z

client-core/src/wallet/syncer.rs

@@ -374,6 +381,52 @@ impl<'a, S: SecureStorage, C: Client, D: TxDecryptor> WalletSyncerImpl<'a, S, C,
        Ok(())
    }

+    fn sync_tx(&mut self, tx_str: &str) -> Result<Coin> {


I guess this is perfectly ok to write as a separate function, it's not "sync".

yihuang · 2019-12-18T12:15:27Z

client-core/src/wallet/syncer.rs

+            spent_flags?,
+        )
+        .chain(|| (ErrorKind::InvalidInput, "import error"))?;
+        self.save(&memento)?;


This is the side-effect of implementing this feature in Syncer, save will also write SyncState which is not used by this feature, but if there is concurrently running syncing process, it will override data.

I'll make an individual function called update_status and use it in save

yihuang · 2019-12-18T12:30:18Z

client-core/src/wallet/syncer_logic.rs

+}
+
+/// update WalletStateMemento when import a transaction
+pub(crate) fn import_transaction(


Maybe we should rename syncer_logic to wallet_logic or something when it's function goes beyond syncing, to prevent future confusion.

yihuang · 2019-12-18T12:43:05Z

client-rpc/src/rpc/wallet_rpc.rs

@@ -64,6 +64,9 @@ pub trait WalletRpc: Send + Sync {

    #[rpc(name = "wallet_transactions")]
    fn transactions(&self, request: WalletRequest) -> Result<Vec<TransactionChange>>;
+
+    #[rpc(name = "wallet_downloadTransaction")]
+    fn plain_transaction(&self, request: WalletRequest, txid: String) -> Result<String>;


How about sync_tx -> importTransaction, downloadTransaction -> exportTransaction.

tomtau

there are some merge conflicts with master

tomtau · 2019-12-19T09:24:58Z

CHANGELOG.md

+## v0.1.1
+
+### Features
+* *client*: export and import transaction -- transaction in which the receiver's view key not included can be exported by a give transaction id (the sender can list transactions and find it), getting a base64 encoded plain transaction string and the receiver can import the string into wallet.


master is already unreleased 0.2, so perhaps no "0.1.1", just move the features paragraph under 0.2

tomtau

it seems it fails to compile after merges

tomtau

looks all right -- can you squash the commits?

the build fail is now due to clippy changes -- I fixed it in f3eed40 so don't worry about that

tomtau · 2019-12-20T04:01:43Z

bors r+

bors · 2019-12-20T04:34:25Z

This PR was included in a batch with a merge conflict, it will be automatically retried

bors · 2019-12-20T05:07:16Z

Merge conflict

solution: - add a interface `export_tx` for sender to download the the transaction - add a interface `import_tx` for receiver to import the transaction and get the output UTXO

devashishdxt · 2019-12-27T03:07:07Z

bors r+

695: Problem:(CRO-557) no way to import transactions (without a view key) r=devashishdxt a=linfeng-crypto solution: - add a interface `export_tx` for sender to get the plain transaction - add a interface `import_tx` for receiver to import the plain transaction and get the output UTXO 740: Problem:(CRO-661) not enough logging in debug enclave execution r=devashishdxt a=linfeng-crypto solution: add some logs. Co-authored-by: linfeng <linfeng@crypto.com>

bors · 2019-12-27T03:56:15Z

Build failed (retrying...)

continuous-integration/travis-ci/push

695: Problem:(CRO-557) no way to import transactions (without a view key) r=devashishdxt a=linfeng-crypto solution: - add a interface `export_tx` for sender to get the plain transaction - add a interface `import_tx` for receiver to import the plain transaction and get the output UTXO Co-authored-by: linfeng <linfeng@crypto.com>

bors · 2019-12-27T04:35:35Z

Build succeeded

linfeng-crypto requested review from calvinaco, devashishdxt and tomtau as code owners December 17, 2019 10:15

tomtau suggested changes Dec 18, 2019

View reviewed changes

linfeng-crypto force-pushed the CRO-557 branch 2 times, most recently from 68d1b35 to 5e01082 Compare December 18, 2019 11:10

yihuang reviewed Dec 18, 2019

View reviewed changes

yihuang approved these changes Dec 19, 2019

View reviewed changes

tomtau self-requested a review December 19, 2019 09:23

tomtau suggested changes Dec 19, 2019

View reviewed changes

linfeng-crypto force-pushed the CRO-557 branch from 7234ab9 to 82e763e Compare December 19, 2019 11:24

tomtau self-requested a review December 20, 2019 01:55

tomtau suggested changes Dec 20, 2019

View reviewed changes

tomtau self-requested a review December 20, 2019 03:18

tomtau suggested changes Dec 20, 2019

View reviewed changes

linfeng-crypto force-pushed the CRO-557 branch from 5590e9b to 6dc50ab Compare December 20, 2019 03:58

linfeng-crypto force-pushed the CRO-557 branch from 6dc50ab to 6207d2c Compare December 21, 2019 14:28

Problem:(CRO-557) no way to import transactions (without a view key)

91afe97

solution: - add a interface `export_tx` for sender to download the the transaction - add a interface `import_tx` for receiver to import the transaction and get the output UTXO

linfeng-crypto force-pushed the CRO-557 branch from 6207d2c to 91afe97 Compare December 23, 2019 08:31

bors bot merged commit 91afe97 into crypto-com:master Dec 27, 2019

tomtau mentioned this pull request Jan 22, 2020

Problem: outdated send your first tx docs crypto-com/thaler-docs#56

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Problem:(CRO-557) no way to import transactions (without a view key) #695

Problem:(CRO-557) no way to import transactions (without a view key) #695

linfeng-crypto commented Dec 17, 2019 •

edited

Loading

codecov bot commented Dec 17, 2019 •

edited

Loading

tomtau left a comment

tomtau Dec 18, 2019

tomtau Dec 18, 2019

tomtau Dec 18, 2019

leejw51crypto Dec 18, 2019 •

edited

Loading

linfeng-crypto Dec 18, 2019

leejw51crypto commented Dec 18, 2019

yihuang Dec 18, 2019 •

edited

Loading

linfeng-crypto Dec 19, 2019

yihuang Dec 18, 2019

yihuang Dec 18, 2019 •

edited

Loading

linfeng-crypto Dec 19, 2019

yihuang Dec 18, 2019

yihuang Dec 18, 2019

linfeng-crypto Dec 19, 2019

tomtau left a comment

tomtau Dec 19, 2019

tomtau left a comment

tomtau left a comment

tomtau commented Dec 20, 2019

bors bot commented Dec 20, 2019

bors bot commented Dec 20, 2019

devashishdxt commented Dec 27, 2019

bors bot commented Dec 27, 2019

bors bot commented Dec 27, 2019

Problem:(CRO-557) no way to import transactions (without a view key) #695

Problem:(CRO-557) no way to import transactions (without a view key) #695

Conversation

linfeng-crypto commented Dec 17, 2019 • edited Loading

codecov bot commented Dec 17, 2019 • edited Loading

Codecov Report

tomtau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejw51crypto Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejw51crypto commented Dec 18, 2019

yihuang Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomtau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomtau left a comment

Choose a reason for hiding this comment

tomtau left a comment

Choose a reason for hiding this comment

tomtau commented Dec 20, 2019

bors bot commented Dec 20, 2019

bors bot commented Dec 20, 2019

Merge conflict

devashishdxt commented Dec 27, 2019

bors bot commented Dec 27, 2019

Build failed (retrying...)

bors bot commented Dec 27, 2019

Build succeeded

linfeng-crypto commented Dec 17, 2019 •

edited

Loading

codecov bot commented Dec 17, 2019 •

edited

Loading

leejw51crypto Dec 18, 2019 •

edited

Loading

yihuang Dec 18, 2019 •

edited

Loading

yihuang Dec 18, 2019 •

edited

Loading