revault / revault-gui

Revault terminal
https://revault.dev
BSD 3-Clause "New" or "Revised" License
51 stars 8 forks source link

Changed `Psbt` to `SpendTransaction`. #368

Closed Zshan0 closed 2 years ago

Zshan0 commented 2 years ago

ManagerImportSendTransactionState currently stores the psbt directly in psbt_imported. This has been updated to SpendTransaction. Following this change is a chain of minor changes under impl State for the same.

The implemented test only checks if the parsing for a psbt string happens through SpendTransaction::from_psbt_str. This PR resolves #79

Zshan0 commented 2 years ago

As suggested, state is being checked with the valid psbt. Since the daemon is currently only dealing with a single transaction, I assumed that setspendtx is being called upon it and the daemon responds with an ok. Let me know if there is something that needs to be fixed. Thanks :D

edouardparis commented 2 years ago

In order to understand what's going on:

The state is ManagerImportTransactionState it cannot be equal to a psbt. You should add a method imported_spend -> &Option<SpendTransaction> and do assert!(state.imported_spend(), spend)

No need to mock ListVault, the state never used this command.

Zshan0 commented 2 years ago

This request set_spend_tx must be mocked with a response.

Since SpendTxMessage::Import triggers update_spend_tx and not set_spend_tx here, should I mock the response for this instead?

edouardparis commented 2 years ago

Yes your code is correct, it is 'updatespendtx' that need to be mocked.

To answer your question, to return a reference you only need to append & to the variable you want to return. https://doc.rust-lang.org/reference/types/pointer.html Why do we introduce a new method imported_tx: it is to expose a reference to the psbt_imported field withoud making the field public. https://doc.rust-lang.org/reference/visibility-and-privacy.html

diff --git a/src/app/state/manager.rs b/src/app/state/manager.rs
index 7df0044..4820092 100644
--- a/src/app/state/manager.rs
+++ b/src/app/state/manager.rs
@@ -459,13 +459,8 @@ impl ManagerImportSendTransactionState {
         SpendTransaction::from_psbt_str(&self.psbt_input.value).ok()
     }

-    pub fn imported_state(&self) -> Option<SpendTransaction> {
-        Some(
-            SpendTransaction::from_raw_psbt(
-                &self.psbt_imported.as_ref().unwrap().as_psbt_serialized(),
-            )
-            .unwrap(),
-        )
+    pub fn imported_tx(&self) -> &Option<SpendTransaction> {
+        &self.psbt_imported
     }
 }

diff --git a/tests/app_manager.rs b/tests/app_manager.rs
index 50b0eed..21153ae 100644
--- a/tests/app_manager.rs
+++ b/tests/app_manager.rs
@@ -188,5 +188,5 @@ async fn test_manager_import_spend() {
         .await;

     let state: &ManagerImportSendTransactionState = sandbox.state();
-    assert_eq!(state.imported_state().unwrap(), spend);
+    assert_eq!(state.imported_tx().as_ref().unwrap(), &spend);
 }