Create a loan backup file when confirming #226

bonomat · 2021-08-05T06:11:06Z

Add loan back up feature to extension.

da-kami · 2021-08-11T10:19:05Z

seems this needs a proper rebase on master - looks like you built on top of Thomas' work but that was force pushed, or are Thomas' commits supposed to be here?

da-kami

Looks good - might want to tell the user that the backup is downloaded as file in the browser downloads. Not sure how feasible that is though.

One thing I find somewhat problematic: The stepper does not keep the state. If the extension is collapsed (also happens when changing focus away from the browser window to check the backup on file system...) - I have to press sign again (i.e. go through all the steps...) - this is a bit weird, because in the end I can actually not check the file because that leads to an endless loop of doing all the steps....

da-kami · 2021-08-11T10:20:03Z

extension/package.json

@@ -46,13 +46,15 @@
    "@types/react": "^16.9.0",
    "@types/react-dom": "^16.9.0",
    "@types/uuid": "^8.3.1",
+    "chakra-ui-steps": "^1.3.0",


noice! I also found that one and wanted to use it at some point :D

extension/wallet/src/wallet/loan_backup.rs

da-kami · 2021-08-11T10:39:07Z

extension/wallet/src/wallet/loan_backup.rs

+    // We get a hold of the wallet to ensure that it is loaded. This is a security mechanism
+    // to ensure no unauthorized access to the data.
+    // Ideally all data is encrypted but that's just how it is :)
+    let _ = current(&name, current_wallet).await.unwrap();


couldn't we encrypt using the wallet's key?

I guess so. I created a ticket for that: #234

If you want the wasm code to blow up less, I would avoid unwraps or at least use expect to document the invariant of why you think it is okay to panic.

bonomat · 2021-08-11T22:39:35Z

One thing I find somewhat problematic: The stepper does not keep the state. If the extension is collapsed (also happens when changing focus away from the browser window to check the backup on file system...) - I have to press sign again (i.e. go through all the steps...) - this is a bit weird, because in the end I can actually not check the file because that leads to an endless loop of doing all the steps...

I know. @thomaseizinger, do you have an idea here?
The issue is that the loan signing process is a 3 step process. If the user interupts it by clicking outside of the window, it starts from scratch.
Shall keep some state here?
A "simple" solution would be to open a popup window. That one will not close if you lose focus.

Signed-off-by: Philipp Hoenisch <philipp@hoenisch.at>

The feature allows you to export the loan details in json format. Signed-off-by: Philipp Hoenisch <philipp@hoenisch.at>

Signed-off-by: Philipp Hoenisch <philipp@hoenisch.at>

bonomat · 2021-08-12T00:07:11Z

One thing I find somewhat problematic: The stepper does not keep the state. If the extension is collapsed (also happens when changing focus away from the browser window to check the backup on file system...) - I have to press sign again (i.e. go through all the steps...) - this is a bit weird, because in the end I can actually not check the file because that leads to an endless loop of doing all the steps...

I know. @thomaseizinger, do you have an idea here?
The issue is that the loan signing process is a 3 step process. If the user interupts it by clicking outside of the window, it starts from scratch.
Shall keep some state here?
A "simple" solution would be to open a popup window. That one will not close if you lose focus.

I created a ticket for this: #235

Signed-off-by: Philipp Hoenisch <philipp@hoenisch.at>

Co-authored-by: Daniel Karzel <daniel.karzel@coblox.tech>

thomaseizinger

I know. @thomaseizinger, do you have an idea here?
The issue is that the loan signing process is a 3 step process. If the user interupts it by clicking outside of the window, it starts from scratch.
Shall keep some state here?

What about the following:

Instead of having backup be a dedicated action, we could add a checkbox like:

Create backup of loan

When ticked, the download will automatically be initiated as part of clicking sign.

thomaseizinger · 2021-08-11T23:42:34Z

extension/wallet/src/wallet/sign_loan.rs

    let borrower = storage
        .get_item::<String>("borrower_state")
        .map_err(Error::Load)?
        .ok_or(Error::EmptyState)?;
    let (borrower, loan_details) =
        serde_json::from_str::<(Borrower1, LoanDetails)>(&borrower).map_err(Error::Deserialize)?;
+    Ok((borrower, loan_details))


When Storage::get_item was designed, the idea was that you get a type-safe interface that builds on top of the Display/FromStr traits to avoid boilerplate.

The repeated use of it with String and serde_json suggests that you actually want an API on Storage that is built on top of a Serialize/Deserialize contract instead.

The benefit of doing so would be that you don't need to extract functions like this because a call to storage::get_item does exactly what you need, resulting in less indirection and easier to read code.

thomaseizinger · 2021-08-11T23:45:43Z

extension/wallet/src/wallet/sign_loan.rs

+    // load temporary loan_borrower state. When the frontend _asks_ the extension to
+    // sign a loan, the information gets stored in the background script first.
+    // When a request to bobtimas was made to actually take the loan,
+    // this temporary loan details are saved in localStorage.
+    // There can only be one pending loans at the time hence there is no identifier.


Given that this is documenting the function, having this as rustdoc on the function would be better.

thomaseizinger · 2021-08-11T23:50:05Z

extension/wallet/src/lib.rs

+#[derive(Debug, thiserror::Error)]
+pub enum Error {
+    #[error("Storage error: {0}")]
+    Storage(anyhow::Error),
+    #[error("Failed to save item to storage: {0}")]
+    Save(anyhow::Error),
+    #[error("Failed to load item from storage: {0}")]
+    Load(anyhow::Error),
+    #[error("Loaded empty borrower state")]
+    EmptyBorrowerState,
+    #[error("Loan details were not found")]
+    LoanNotFound,
+    #[error("Serialization failed: {0}")]
+    Serialize(serde_json::Error),
+    #[error("Deserialization failed: {0}")]
+    Deserialize(serde_json::Error),
+    #[error("Loaded empty borrower state")]
+    EmptyState,
+    #[error("Failed to sign transaction: {0}")]
+    Sign(anyhow::Error),
+}


As outlined in #232, the current state of error isn't particularly ideal. I'd suggest to give it some thought and decide on a strategy so you get to a place where the way things are done in this component is generally applicable and can hence effortlessly "copy" the idea and apply it to new code.

thomaseizinger · 2021-08-11T23:57:01Z

extension/wallet/src/lib.rs

+/// Create a json object which can be used to backup the loan details to the transaction
+///
+/// TODO: Pass in transaction id instead of transaction
+/// For now, we pass in the whole transaction just to access it's transaction id.
+/// This is because `sign_loan` returns the whole transaction and we have no means to deserialize
+/// the transaction from within TS.
+#[wasm_bindgen]
+pub async fn create_loan_backup(
+    wallet_name: String,
+    transaction: JsValue,
+) -> Result<JsValue, JsValue> {
+    log::debug!("Received tx: {:?}", transaction);
+    let transaction: Transaction = map_err_from_anyhow!(transaction.into_serde())?;
+    let backup_details = map_err_from_anyhow!(
+        wallet::create_loan_backup(wallet_name, &LOADED_WALLET, transaction.inner.txid()).await
+    )?;
+    let backup_details = map_err_from_anyhow!(JsValue::from_serde(&backup_details))?;
+    Ok(backup_details)
+}
+
+/// Load a loan backup and store it in localStorage.
+#[wasm_bindgen]
+pub async fn load_loan_backup(backup_details: JsValue) -> Result<(), JsValue> {
+    let backup_details: BackupDetails = map_err_from_anyhow!(backup_details.into_serde())?;
+    map_err_from_anyhow!(wallet::load_loan_backup(backup_details))?;
+
+    Ok(())
+}


The extension frontend TS code should be able to access the same localStorage right?

I think we should consider just creating the backup from within TS. We don't need the wallet for anything to create the backup, so the indirection through the Rust code just feels like unnecessary boilerplate. (On the same note, calling into Rust for creating the seed-words is similarly unnecessary.)

In the long-run, we need to consider that the extension is given data that was created with a previous version and hence be more explicit about what we store rather than just dumping everything.

I wonder if it is a good idea to call things directly from TS code instead.
Are you not afraid that the TS code and rust code diverge too much?

For example, the loan data is currently stored in localstorage under the key loan_state. If this ever to changes in the rust code we might run in the danger that it's not changed in the TS code.

I looked into it: in this particular case it's unfortunately a bigger task :

The issue here is that we need to know the transaction ID to load this information from the localstorage. All we have available is the whole transaction. We'd need to change quite a few things for this. It's worth it and should be done but since it's working atm I recorded an issue for this. #238

extension/wallet/src/lib.rs

thomaseizinger · 2021-08-11T23:58:07Z

extension/wallet/src/lib.rs

+#[wasm_bindgen]
+pub async fn create_loan_backup(
+    wallet_name: String,
+    transaction: JsValue,


Is this passed around using the hex-encoded binary representation? If yes, why isn't it just a String? If not, why not?

Good question. It's a mix right now:

{ inner: "02000...." }

As far as I remember we had challenges to serialize this to a simple string.

thomaseizinger · 2021-08-11T23:59:43Z

extension/wallet/src/wallet/loan_backup.rs

+    // We get a hold of the wallet to ensure that it is loaded. This is a security mechanism
+    // to ensure no unauthorized access to the data.
+    // Ideally all data is encrypted but that's just how it is :)
+    let _ = current(&name, current_wallet).await.unwrap();


If you want the wasm code to blow up less, I would avoid unwraps or at least use expect to document the invariant of why you think it is okay to panic.

thomaseizinger · 2021-08-12T00:02:40Z

extension/src/background/index.ts

-        .then(cleanupPendingLoan);
+
+    // if we receive an error, we respond directly, else we return the details
+    return await signLoan(walletName).catch(rejectLoanSignRequest);


return await is unnecessary, you can just remove the await and return the promise directly.

thomaseizinger · 2021-08-12T00:07:03Z

extension/src/components/ConfirmLoanWizard.tsx

-                            onSigned={onSigned}
-                        />
+                        <form
+                            onSubmit={async e => {


Doesn't need to be async. I think there is a typescript lint for such cases!

There are quite a lint errors. I created a ticket for that: #236

thomaseizinger · 2021-08-12T00:08:00Z

extension/src/components/ConfirmLoanWizard.tsx

-            // Note: it would be nicer to open a dialog to download the file. However, we would lose focus
-            // of the window. We don't want that, hence we just open a new tab with the content
-            window.open(url, "_blank");


Nice!

This commit is candidate for squashing into the earlier one!

I can recommend looking into https://github.com/tummychow/git-absorb for a semi-automated solution to this.

thomaseizinger · 2021-08-12T00:17:51Z

Whoops, looks like my review is coming a bit too late, sorry about that, I only got a notification this morning!

In any case, I didn't see anything major that would have need to be addressed.

On a stylistic note: The guide we are striving to follow for commit messages avoids dots at the end to a) save space and b) be consistent with git's internal commit messages like merges.

bonomat · 2021-08-12T00:19:37Z

Whoops, looks like my review is coming a bit too late, sorry about that, I only got a notification this morning!

In any case, I didn't see anything major that would have need to be addressed.

On a stylistic note: The guide we are striving to follow for commit messages avoids dots at the end to a) save space and b) be consistent with git's internal commit messages like merges.

Sorry for merging so fast. was just in a call with Daniel and we decided to merge. I'll have a look at your feedback and get this sorted in follow-up PR(s).

thomaseizinger · 2021-08-12T00:32:14Z

Whoops, looks like my review is coming a bit too late, sorry about that, I only got a notification this morning!
In any case, I didn't see anything major that would have need to be addressed.
On a stylistic note: The guide we are striving to follow for commit messages avoids dots at the end to a) save space and b) be consistent with git's internal commit messages like merges.

Sorry for merging so fast. was just in a call with Daniel and we decided to merge. I'll have a look at your feedback and get this sorted in follow-up PR(s).

No sweat, take it as you wish :)

bonomat marked this pull request as draft August 5, 2021 06:11

bonomat force-pushed the lending-backup branch from 23c3543 to e707fc2 Compare August 6, 2021 00:37

bonomat changed the title ~~[WIP] Create a loan backup file when confirming~~ Create a loan backup file when confirming Aug 8, 2021

bonomat requested a review from da-kami August 8, 2021 22:50

bonomat marked this pull request as ready for review August 8, 2021 23:27

bonomat force-pushed the lending-backup branch from d5756b0 to 8db5a7a Compare August 11, 2021 05:09

da-kami reviewed Aug 11, 2021

View reviewed changes

bonomat added 4 commits August 12, 2021 08:40

Split up sign_loan function into logical parts.

72da67e

Signed-off-by: Philipp Hoenisch <philipp@hoenisch.at>

Add loan backup feature.

17dbbdc

The feature allows you to export the loan details in json format. Signed-off-by: Philipp Hoenisch <philipp@hoenisch.at>

Add load loan backup feature into option page.

2be7a09

Signed-off-by: Philipp Hoenisch <philipp@hoenisch.at>

Clean up confirm loan by merging the files.

2b21b57

Signed-off-by: Philipp Hoenisch <philipp@hoenisch.at>

bonomat force-pushed the lending-backup branch from 8db5a7a to ebfe3fa Compare August 11, 2021 22:40

bonomat mentioned this pull request Aug 12, 2021

State in extension popup #235

Open

bonomat and others added 2 commits August 12, 2021 10:08

Download file instead of opening a new tab.

3a9e86c

Signed-off-by: Philipp Hoenisch <philipp@hoenisch.at>

Update extension/wallet/src/wallet/loan_backup.rs

933758c

Co-authored-by: Daniel Karzel <daniel.karzel@coblox.tech>

bonomat force-pushed the lending-backup branch from deec242 to 933758c Compare August 12, 2021 00:09

bonomat merged commit 638cd4e into master Aug 12, 2021

bonomat deleted the lending-backup branch August 12, 2021 00:12

bonomat mentioned this pull request Aug 12, 2021

[Extension] Loan Backup and Recovery #215

Closed

thomaseizinger reviewed Aug 12, 2021

View reviewed changes

This was referenced Aug 12, 2021

Introduce tslint for frontend and extension #236

Open

The extension TS code can access localStorage directly #238

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a loan backup file when confirming #226

Create a loan backup file when confirming #226

bonomat commented Aug 5, 2021 •

edited

Loading

da-kami commented Aug 11, 2021

da-kami left a comment •

edited

Loading

da-kami Aug 11, 2021

da-kami Aug 11, 2021

bonomat Aug 11, 2021

thomaseizinger Aug 11, 2021

bonomat commented Aug 11, 2021

bonomat commented Aug 12, 2021

thomaseizinger left a comment •

edited by bonomat

Loading

thomaseizinger Aug 11, 2021

thomaseizinger Aug 11, 2021

thomaseizinger Aug 11, 2021

thomaseizinger Aug 11, 2021

thomaseizinger Aug 11, 2021

bonomat Aug 12, 2021

bonomat Aug 12, 2021

thomaseizinger Aug 11, 2021

bonomat Aug 12, 2021

thomaseizinger Aug 11, 2021

thomaseizinger Aug 12, 2021

thomaseizinger Aug 12, 2021

bonomat Aug 12, 2021

thomaseizinger Aug 12, 2021

thomaseizinger Aug 12, 2021

thomaseizinger commented Aug 12, 2021

bonomat commented Aug 12, 2021

thomaseizinger commented Aug 12, 2021

Create a loan backup file when confirming #226

Create a loan backup file when confirming #226

Conversation

bonomat commented Aug 5, 2021 • edited Loading

da-kami commented Aug 11, 2021

da-kami left a comment • 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

bonomat commented Aug 11, 2021

bonomat commented Aug 12, 2021

thomaseizinger left a comment • edited by bonomat 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

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

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

Choose a reason for hiding this comment

thomaseizinger commented Aug 12, 2021

bonomat commented Aug 12, 2021

thomaseizinger commented Aug 12, 2021

bonomat commented Aug 5, 2021 •

edited

Loading

da-kami left a comment •

edited

Loading

thomaseizinger left a comment •

edited by bonomat

Loading