Database dependency injection in planner #1427

grod220 · 2024-07-05T14:32:17Z

While working on #1426, it became evident that the state of testing in the wasm code is quite rough given the dependency on the indexeddb global.

This PR adds a database interface that storage.rs gets passed upon initialization. This allows us to use a mock database that can be used in tests, making our goals around test coverage much more attainable.

changeset-bot · 2024-07-05T14:32:20Z

🦋 Changeset detected

Latest commit: 86a2dca

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages

Name	Type
@penumbra-zone/wasm	Minor
@penumbra-zone/perspective	Major
@penumbra-zone/query	Major
@penumbra-zone/services	Major
@penumbra-zone/storage	Major
minifront	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

grod220 · 2024-07-08T13:11:53Z

packages/wasm/crate/src/database/indexed_db.rs

+async fn mock_test_database(mut db_req: OpenDbRequest) -> OpenDbRequest {
+    db_req.set_on_upgrade_needed(Some(|evt: &IdbVersionChangeEvent| -> Result<(), JsValue> {
+        // Check if the object store exists; create it if it doesn't
+        if evt.db().name() == "penumbra-db-wasm-test" {
+            let note_key: JsValue = serde_wasm_bindgen::to_value("noteCommitment.inner")?;
+            let note_object_store_params = IdbObjectStoreParameters::new()
+                .key_path(Some(&IdbKeyPath::new(note_key)))
+                .to_owned();
+            let note_object_store = evt
+                .db()
+                .create_object_store_with_params("SPENDABLE_NOTES", &note_object_store_params)?;
+
+            let nullifier_key: JsValue = serde_wasm_bindgen::to_value("nullifier.inner")?;
+            note_object_store.create_index_with_params(
+                "nullifier",
+                &IdbKeyPath::new(nullifier_key),
+                web_sys::IdbIndexParameters::new().unique(false),
+            )?;
+            evt.db().create_object_store("TREE_LAST_POSITION")?;
+            evt.db().create_object_store("TREE_LAST_FORGOTTEN")?;
+
+            let commitment_key: JsValue = serde_wasm_bindgen::to_value("commitment.inner")?;
+            let commitment_object_store_params = IdbObjectStoreParameters::new()
+                .key_path(Some(&IdbKeyPath::new(commitment_key)))
+                .to_owned();
+            evt.db().create_object_store_with_params(
+                "TREE_COMMITMENTS",
+                &commitment_object_store_params,
+            )?;
+            evt.db().create_object_store("TREE_HASHES")?;
+            evt.db().create_object_store("FMD_PARAMETERS")?;
+            evt.db().create_object_store("APP_PARAMETERS")?;
+            evt.db().create_object_store("GAS_PRICES")?;
+        }
+        Ok(())
+    }));
+
+    db_req
+}


Copied over 1:1. We should seek in later PRs to migrate this to MockDb.

grod220 · 2024-07-08T13:13:04Z

packages/wasm/crate/src/database/interface.rs

The core idea of this PR is this interface which allows us to write a mock and write tests that feel much more manageable to write

grod220 · 2024-07-08T13:14:33Z

packages/wasm/crate/src/planner.rs

-    let gas_prices: GasPrices = {
-        let gas_prices: penumbra_proto::core::component::fee::v1::GasPrices =
-            serde_wasm_bindgen::from_value(
-                storage
-                    .get_gas_prices_by_asset_id(fee_asset_id)
-                    .await?
-                    .ok_or_else(|| anyhow!("GasPrices not available"))?,
-            )?;
-        gas_prices.try_into()?
-    };


Using the serialization built into the method now

grod220 · 2024-07-08T13:14:42Z

packages/wasm/crate/src/planner.rs

@@ -463,7 +462,7 @@ pub async fn plan_transaction(

        save_auction_nft_metadata_if_needed(auction_id, &storage, seq).await?;
        let outstanding_reserves: OutstandingReserves =
-            storage.get_auction_oustanding_reserves(auction_id).await?;


grod220 · 2024-07-08T13:16:11Z

packages/wasm/crate/tests/test_mock_db.rs

An example test suite, but the next PR is to implement a test suite for get_notes_for_voting()

Valentine1898

LGTM! 👍

TalDerei · 2024-07-08T14:37:20Z

reviewing the PR now 👍

TalDerei

This is a huge improvement to our storage crate, and it's more readable! 🎉

TalDerei · 2024-07-08T18:54:43Z

packages/wasm/crate/src/database/mock.rs

+        T: DeserializeOwned,
+        K: Into<JsValue>,
+    {
+        unimplemented!()


ideally we should avoid leaving methods unimplemented, and explicitly state why they are unimplemented.

Added comments. In short, the person writing the tests that use these methods will have to write them. Figured it's a task for later.

TalDerei · 2024-07-08T19:32:56Z

packages/wasm/crate/src/database/indexed_db.rs

+// Previous method of testing that requires features in prod code to seed indexed db for tests
+// TODO: Swap out with new MockDb utility for testing
+async fn mock_test_database(mut db_req: OpenDbRequest) -> OpenDbRequest {
+    db_req.set_on_upgrade_needed(Some(|evt: &IdbVersionChangeEvent| -> Result<(), JsValue> {


tagging penumbra-zone/penumbra#3289 (comment) as relevant discussion.

how did you resolve the onupgradeneeded conflict I was facing?

In short, I didn't. By passing in an alternative database, the test suite has full flexibility in instantiating it as it needs.

TalDerei · 2024-07-08T20:00:43Z

packages/wasm/crate/src/planner.rs

@@ -114,7 +115,7 @@ async fn save_unbonding_token_metadata_if_needed(
 /// action renders correctly in the transaction approval dialog.
 async fn save_auction_nft_metadata_if_needed(
    id: AuctionId,
-    storage: &IndexedDBStorage,
+    storage: &Storage<IdbDatabase>,


I really like the generic storage interface!

TalDerei · 2024-07-08T20:02:39Z

packages/wasm/crate/tests/test_mock_db.rs

+    let key = "test_key";
+    let value = AddressIndex::new(1);
+
+    block_on(db.put_with_key(table_name, key, &value)).unwrap();


how does block_on work in the test suite?

I didn't occur to me that I could just define the test fn as async and use await syntax. This was an unnecessary workaround. Will update.

grod220 force-pushed the db-abstraction branch from 56b5b56 to ad19b7a Compare July 5, 2024 14:38

grod220 requested review from Valentine1898, cratelyn and TalDerei July 5, 2024 14:39

grod220 force-pushed the db-abstraction branch 4 times, most recently from 5502149 to 737e918 Compare July 8, 2024 13:10

grod220 commented Jul 8, 2024

View reviewed changes

grod220 self-assigned this Jul 8, 2024

Valentine1898 approved these changes Jul 8, 2024

View reviewed changes

TalDerei approved these changes Jul 8, 2024

View reviewed changes

grod220 added 3 commits July 8, 2024 23:57

Database dependency injection in planner

ed2398e

Update interface

6ccd7bd

review updates

2159161

grod220 force-pushed the db-abstraction branch from 737e918 to 2159161 Compare July 8, 2024 22:22

correct tables

86a2dca

grod220 merged commit 5641af2 into main Jul 8, 2024
6 checks passed

grod220 deleted the db-abstraction branch July 8, 2024 22:57

grod220 mentioned this pull request Jul 15, 2024

Delegator voting tests #1480

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Database dependency injection in planner #1427

Database dependency injection in planner #1427

grod220 commented Jul 5, 2024

changeset-bot bot commented Jul 5, 2024 •

edited

Loading

grod220 Jul 8, 2024

grod220 Jul 8, 2024

grod220 Jul 8, 2024

grod220 Jul 8, 2024

grod220 Jul 8, 2024

Valentine1898 left a comment

TalDerei commented Jul 8, 2024

TalDerei left a comment •

edited

Loading

TalDerei Jul 8, 2024 •

edited

Loading

grod220 Jul 8, 2024

TalDerei Jul 8, 2024

grod220 Jul 8, 2024

TalDerei Jul 8, 2024

TalDerei Jul 8, 2024 •

edited

Loading

grod220 Jul 8, 2024

Database dependency injection in planner #1427

Database dependency injection in planner #1427

Conversation

grod220 commented Jul 5, 2024

changeset-bot bot commented Jul 5, 2024 • edited Loading

🦋 Changeset detected

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

Valentine1898 left a comment

Choose a reason for hiding this comment

TalDerei commented Jul 8, 2024

TalDerei left a comment • edited Loading

Choose a reason for hiding this comment

TalDerei Jul 8, 2024 • 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

TalDerei Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Jul 5, 2024 •

edited

Loading

TalDerei left a comment •

edited

Loading

TalDerei Jul 8, 2024 •

edited

Loading

TalDerei Jul 8, 2024 •

edited

Loading