Improvements and fixes to wasm transaction planner and wasm view server #2973

zpoken · 2023-09-06T15:24:01Z

added wasm planner which allows more flexible creation of TransactionPlan on TS side
added logic of storing and reading advice when scanning blocks
added function for generating ephemeral address
planner reads data from indexedDB directly from rust

grod220

Please also run rustfmt and clippy on the wasm crate 🙏

grod220 · 2023-09-06T19:01:21Z

crates/wasm/Cargo.toml

@@ -39,6 +39,10 @@ serde-wasm-bindgen       = "0.5.0"
 wasm-bindgen             = "0.2.87"
 wasm-bindgen-futures     = "0.4.37"
 web-sys                  = { version = "0.3.64", features = ["console"] }
+tracing = "0.1"


grod220 · 2023-09-06T19:03:30Z

crates/wasm/src/lib.rs

+        .context("The provided string is not a valid FullViewingKey")
+        .unwrap();
+
+    let (address, _dtk) = fvk.ephemeral_address(OsRng, index.into());


How does OsRng work in the web context? Does it just work?

Yeah, I believe the crate has compile-time features that hook the platform APIs.

grod220 · 2023-09-06T19:18:05Z

crates/wasm/src/lib.rs

@@ -74,6 +77,20 @@ pub fn get_address_by_index(full_viewing_key: &str, index: u32) -> JsValue {
    return serde_wasm_bindgen::to_value(&address_str).unwrap();
 }

+#[wasm_bindgen]
+pub fn get_ephemeral_address(full_viewing_key: &str, index: u32) -> JsValue {


JsValue is quite painful trying to understand what comes back from this function. We also need to start adding docstrings above every imported function so it helps the frontend more. Something like:

/// Generates a kind of "burner" address in which the user can use to maintain their anonymity without sharing their main address. /// Returns: `u128` #[wasm_bindgen] pub fn get_ephemeral_address(full_viewing_key: &str, index: u32) -> JsValue {

and this would work for parameters too:

/// Scans block for notes, swaps... /// Arguments: /// compact_block: `v1alpha1::CompactBlock` /// Returns: `ScanBlockResult` #[wasm_bindgen] pub fn scan_block_without_updates(&mut self, compact_block: JsValue) -> JsValue {

grod220 · 2023-09-06T19:20:02Z

crates/wasm/src/planner.rs


    /// Set the expiry height for the transaction plan.
+    #[instrument(skip(self))]


What is this and what is it used for?

grod220 · 2023-09-06T19:22:03Z

crates/wasm/src/planner.rs

+    // pub async fn plan(
+    //     &mut self,
+    //     account_group_id: AccountGroupId,
+    //     source: AddressIndex,
+    //     self_address: Address
+    // ) -> anyhow::Result<TransactionPlan> {
+    //     // Gather all the information needed from the view service
+    //     let chain_params: ChainParameters = Default::default();
+    //     let fmd_params: FmdParameters =  Default::default();
+    //     let mut spendable_notes = Vec::new();
+    //     let mut voting_notes = Vec::new();
+    //     // let (spendable_requests, voting_requests) = self.notes_requests(account_group_id, source);
+    //
+    //
+    //     // Plan the transaction using the gathered information
+    //
+    //     self.plan_with_spendable_and_votable_notes(
+    //         &chain_params,
+    //         &fmd_params,
+    //         spendable_notes,
+    //         voting_notes,
+    //         self_address,
+    //     )
+    // }


grod220 · 2023-09-06T19:34:19Z

crates/wasm/src/view_server.rs

+    let tx = db.transaction_on_one("notes").ok().unwrap();
+    let store = tx.object_store("notes").ok().unwrap();


I noticed we use a lot of unwrap()'s. Also these patterns:

.ok()?.await.ok()?; .ok().unwrap()

In general, we need to move to the standard Result pattern and not use unwraps unless they are right as we coerce them to a JsValue and encode them for javascript.

Also, doing .ok() on everything has downsides:

We lose error information, making debugging harder

? is usually used on Result types. Using it on an Option is confusing.

This will cause a panic if a None is received.

We are treating Options as Results.

In most cases we will have to use a rather massive construction with map_err

.map_err(|err| JsError::new(&err.to_string()))?

On this issue , there is no more ergonomic solution yet

grod220 · 2023-09-06T19:38:46Z

crates/wasm/src/wasm_planner.rs

+        input_value: JsValue,
+        into_denom: JsValue,
+        swap_claim_fee: JsValue,
+        claim_address: JsValue,


Can't these be?:

input_value: u128, into_denom: &str, swap_claim_fee: u128, claim_address: &str,

Whenever possible, we should not do JsValue if it is allowed. You may have to test this.

Same feedback as before 👆

The point is that on the TS side we will be using mostly proto types to interact with wasm.
And in this case, having an object of the Address type on the TS side, we will need to

convert Address -> string on the TS side

on the wasm side get an Address from string object
This leads to additional writing of specific conversion code, while the use of JsValue will be unified everywhere else

grod220 · 2023-09-06T19:40:03Z

crates/wasm/src/wasm_planner.rs

+        let _ = self.planner.swap(
+            input_value_proto.try_into().unwrap(),
+            into_denom_proto.try_into().unwrap(),
+            swap_claim_fee_proto.try_into().unwrap(),
+            claim_address_proto.try_into().unwrap(),
+        );
+
+        Ok(())


What's the purpose of this function?

This is part of the functionality for flexible TransactionPlan creation
An example of use can be found here
With this function we add Swap actions to planner

grod220 · 2023-09-06T19:41:11Z

crates/wasm/src/wasm_planner.rs

+}
+
+pub async fn get_chain_parameters() -> Option<ChainParameters> {
+    let db_req: OpenDbRequest = IdbDatabase::open_u32("penumbra", 12).ok()?;


I think we should have a IndexDb struct that has all of these helper functions in one place in another file.

conorsch · 2023-09-08T17:36:10Z

Discussed during sprint planning meeting today. We're currently planning to release in one business day, on Monday, #2984. Is this PR QOL, meaning it's OK to let slip for Monday? @grod220 will ping Zpoken folks to get a sense of urgency. It's technically still possible to dogpile on this and get it in for Monday if necessary.

grod220 · 2023-09-12T10:21:42Z

Take a look at this example. At the top level, all #[wasm_bindgen] functions need to return Result<JsValue, Error> if consuming functions that return results or just JsValue if not using any functions that that return results (should be very rare).

However, every other function in this crate should be using the Result type (in my example the custom WasmResult). This example uses thiserror crate to handle the mapping of all possible errors in the code. This means, error messages are retained, you can use the succinct ? without mapping errors, and the code is responsible for result handling like normal Rust code.

use std::str::FromStr;

use indexed_db_futures::IdbDatabase;
use indexed_db_futures::prelude::OpenDbRequest;
use serde_wasm_bindgen::Error;
use thiserror::Error;
use wasm_bindgen::JsValue;
use wasm_bindgen::prelude::wasm_bindgen;
use web_sys::DomException;

use penumbra_keys::keys::{SeedPhrase, SpendKey};
use penumbra_proto::DomainType;
use penumbra_proto::serializers::bech32str;

// --------------- //
// DomException doesn't play nicely with thiserror's automated tooling and needs to be wrapped

#[derive(Debug)]
pub struct DomError(DomException);

impl std::fmt::Display for DomError {
    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
        write!(f, "DOM Exception: {:?}", self.0)
    }
}

impl std::error::Error for DomError {}

impl From<DomException> for WasmError {
    fn from(dom_exception: DomException) -> Self {
        WasmError::Dom(DomError(dom_exception))
    }
}
// --------------- //

pub type WasmResult<T> = Result<T, WasmError>;


#[derive(Error, Debug)]
pub enum WasmError {
    #[error("{0}")]
    Anyhow(#[from] anyhow::Error),

    #[error("{0}")]
    Dom(#[from] DomError),
}

impl From<WasmError> for serde_wasm_bindgen::Error {
    fn from(wasm_err: WasmError) -> Self {
        Error::new(wasm_err.to_string())
    }
}

#[wasm_bindgen]
pub async fn some_wasm_fn() -> Result<JsValue, Error> {
    let result = get_spend_key().await?;
    Ok(JsValue::from_str(&result))
}

pub async fn get_spend_key() -> WasmResult<String> {
    let db_req: OpenDbRequest = IdbDatabase::open_u32("penumbra", 12)?;
    // do some stuff
    let spend_key = spend_key_new("xyz")?;
    Ok(spend_key)
}


pub fn spend_key_new(seed_phrase: &str) -> WasmResult<String> {
    let seed = SeedPhrase::from_str(seed_phrase)?;
    let spend_key = SpendKey::from_seed_phrase(seed, 0);

    let proto = spend_key.to_proto();
    Ok(bech32str::encode(
        &proto.inner,
        bech32str::spend_key::BECH32_PREFIX,
        bech32str::Bech32m,
    ))
}

Valentine1898 · 2023-09-18T19:37:26Z

@grod220
I've completed a major refactoring of wasm

added documentation for all wasm_bindgen functions
unwrap() and ok() constructs are no longer used
all indexedDB interactions have been moved to a separate file, and some of the code is being reused

Valentine1898 · 2023-09-18T21:33:31Z

@zbuc
In this PR, I have already gotten rid of using unwrap() in wasm crate
I suggest leaving my variant as it uses succinct ? and allows for proper error handling in JS

grod220

Substantially better and more succinct with Error handling now fixed 🎉

Some additional comments. Let me know if you wanted to pair on this to make the remaining fixes or if you wanted to continue on your own.

grod220 · 2023-09-12T09:04:47Z

crates/wasm/src/storage.rs

+impl Store {
+    fn as_str(&self) -> &'static str {
+        match *self {
+            // *self has type Direction


What does type Direction mean?

I removed it even earlier

grod220 · 2023-09-19T05:40:33Z

crates/wasm/Cargo.toml

+ark-ff = { version = "0.4", features = ["std"] }
+decaf377 = { version = "0.5", features = ["r1cs"] }
+thiserror = "1.0"
+wasm-bindgen-test = "0.3.0"


A few things. Can you:

Align the = like the dependencies above

Put in alphabetical order

Put the fully qualified version (example: instead of 0.4, make it 0.4.2)

Align and alphabetical order ✅
As for the full version, I use the version that uses in other penumbra crates

grod220 · 2023-09-19T05:41:27Z

crates/wasm/Cargo.toml

+
+
+



You only need one space between blocks

grod220 · 2023-09-19T05:42:44Z

crates/wasm/publish/publish-wasm.ts

crates/wasm/publish/ directory is critical for the CI/CD to publish new packages on every version bump. Please store these files.

grod220 · 2023-09-19T05:43:57Z

crates/wasm/src/error.rs

+#[derive(Debug)]
+pub struct DomError(DomException);
+
+impl std::fmt::Display for DomError {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        write!(f, "DOM Exception: {:?}", self.0)
+    }
+}
+
+impl std::error::Error for DomError {}
+
+impl From<DomException> for WasmError {
+    fn from(dom_exception: DomException) -> Self {
+        WasmError::Dom(DomError(dom_exception))
+    }
+}
+// --------------- //


Can you put these at the bottom of the file? Would be nice if WasmResult/WasmError were at the top.

grod220 · 2023-09-19T06:07:50Z

crates/wasm/src/wasm_planner.rs

+    /// Arguments:
+    ///     memo: `MemoPlaintext`
+    pub fn memo(&mut self, memo: JsValue) -> Result<(), Error> {
+        self.memo_inner(memo)?;


Same feedback as the others. I'm not sure the point of having inner helper functions here given the body of the helper function is so small.

This is only necessary in order to use WasmResult<()> in the helper function

In that case we should do this:

// In wasm_planner.rs pub fn memo(&mut self, memo: JsValue) -> WasmResult<()> { let memo_proto: MemoPlaintext = serde_wasm_bindgen::from_value(memo)?; let _ = self.planner.memo(memo_proto.try_into()?); Ok(()) } // in error.rs impl From<WasmError> for JsValue { fn from(error: WasmError) -> Self { JsError::from(error).into() } }

And then we can use WasmResult everywhere, even the #[wasm_bindgen] top level functions. It will remove the need of using the inner pattern too. Helper functions are not bad, but we should use them when necessary and not to please the wasm_bindgen gods.

I removed some of the helper functions that seemed redundant. ✅

grod220 · 2023-09-19T06:08:30Z

crates/wasm/src/wasm_planner.rs

+        input_value: JsValue,
+        into_denom: JsValue,
+        swap_claim_fee: JsValue,
+        claim_address: JsValue,


Same feedback as before 👆

grod220 · 2023-09-19T06:10:22Z

crates/wasm/src/wasm_planner.rs

+            .storage
+            .get_swap_by_commitment(swap_commitment_proto)
+            .await?
+            .expect("Swap record not found")


If you are doing .expect() on these, why not just have get_swap_by_commitment return a Result instead of an Option?

grod220 · 2023-09-19T06:11:52Z

crates/wasm/src/storage.rs

+            .await?
+            .map(serde_wasm_bindgen::from_value)
+            .transpose()
+            .map_err(WasmError::from)


With thiserror, you shouldn't need to be doing .map_err(WasmError::from) anymore. Just put the error in the new Error struct.

grod220 · 2023-09-19T06:12:20Z

crates/wasm/src/storage.rs

+            .get_owned("chain_parameters")?
+            .await?
+            .map(serde_wasm_bindgen::from_value)
+            .transpose()


I would not convert this to an Option as every consumer is treating it as a Result.

Same feedback for the rest of these as well

I proceeded from the fact that the case when we successfully read data from indexedDb by a given key, but there was no record by this key in indexedDB, should not lead to an error.

It feels easier to make this a Result as we are nearly always expecting data to be there. It looks like there is only one Option case that could easily work the same as a Result. If you feel strongly otherwise, I can live with it though.

Another point is that when initially reading from indexedDB, we get Option<JsValue>

let read_result: Option<JsValue> = store .get_owned("chain_parameters")? .await?;

This means we'd need to convert Option -> Result, and it doesn't seem to be very convenient
For some functions like get_chain_parameters() and get_fmd_parameters(), we do expect that either the record exists or something is wrong.
But in other cases, there is a high probability that we will successfully read from indexedDb, but the data will not be there. And in the future, if we want more interaction with indexedDb, there will be more logic similar to

let option = storage.get_note_by_nullifier().await?; match option { None => { // forget nullifier } Some(note_record) => { // Mark record_note as spent } }

grod220 · 2023-09-19T06:16:18Z

Also, it looks like there are some merge conflicts with main. Please rebase/merge to fix 🙏

Valentine1898 · 2023-09-19T09:43:06Z

Substantially better and more succinct with Error handling now fixed 🎉

Some additional comments. Let me know if you wanted to pair on this to make the remaining fixes or if you wanted to continue on your own.

I think I can make the fixes myself

Valentine1898 · 2023-09-19T10:07:00Z

crates/wasm/src/tx.rs

+                // An error here indicates we don't know the nullifier, so we omit it from the Perspective.
+                if let Some(spendable_note_record) =
+                    storage.get_note_by_nullifier(&nullifier).await?
+                {
+                    txp.spend_nullifiers
+                        .insert(nullifier, spendable_note_record.note.clone());
+                }


For example, Option is used here when indexedDB reading

Valentine1898 · 2023-09-19T11:40:33Z

I think I've fixed all the comments except those related to these two issues

Should functions reading from indexedDb return WasmResult<Option<SwapRecord>>
most helper functions are meaningless, they are only needed so that #[wasm_bindgen] functions return Result<JsValue, Error> and we can use succinct ?
@grod220

grod220 · 2023-09-19T12:25:46Z

crates/wasm/Cargo.toml

@@ -29,15 +29,19 @@ penumbra-tct           = { path = "../crypto/tct" }
 penumbra-transaction   = { path = "../core/transaction", default-features = false }

 anyhow                   = "1.0.75"
+ark-ff                   = { version = "0.4", features = ["std"] }


Please use the fully qualified version here. Though it's commonly used shortened in this repo, it's actually a mistake. By not specifying the fully qualified path, there is a chance a developer may use and older version than the specific version we tested on our machines. It will always be able to go higher, that's ok. But, we do not want it to go lower.

More information on this: https://users.rust-lang.org/t/psa-please-specify-precise-dependency-versions-in-cargo-toml/71277

I agree, fixed it

grod220 · 2023-09-19T12:26:03Z

crates/wasm/src/keys.rs

+/// get address by index using FVK
+/// Arguments:
+///     full_viewing_key: `bech32 string`
+///     index: `u32`
+/// Returns: `pb::Address`


Amazing 🎉

grod220 · 2023-09-19T12:31:13Z

crates/wasm/src/storage.rs

+            .get_owned("chain_parameters")?
+            .await?
+            .map(serde_wasm_bindgen::from_value)
+            .transpose()


It feels easier to make this a Result as we are nearly always expecting data to be there. It looks like there is only one Option case that could easily work the same as a Result. If you feel strongly otherwise, I can live with it though.

grod220 · 2023-09-19T12:32:06Z

crates/wasm/src/tx.rs

+    serde_wasm_bindgen::to_value(&response)
+}
+
+/// deprecated


Can you add that context?

/// TODO: Deprecated. Still used in `penumbra-zone/wallet`, remove when migration is complete.

grod220 · 2023-09-19T12:34:53Z

crates/wasm/src/wasm_planner.rs

+    /// Arguments:
+    ///     memo: `MemoPlaintext`
+    pub fn memo(&mut self, memo: JsValue) -> Result<(), Error> {
+        self.memo_inner(memo)?;


In that case we should do this:

// In wasm_planner.rs pub fn memo(&mut self, memo: JsValue) -> WasmResult<()> { let memo_proto: MemoPlaintext = serde_wasm_bindgen::from_value(memo)?; let _ = self.planner.memo(memo_proto.try_into()?); Ok(()) } // in error.rs impl From<WasmError> for JsValue { fn from(error: WasmError) -> Self { JsError::from(error).into() } }

And then we can use WasmResult everywhere, even the #[wasm_bindgen] top level functions. It will remove the need of using the inner pattern too. Helper functions are not bad, but we should use them when necessary and not to please the wasm_bindgen gods.

grod220

Wow! 🎊

Though this PR took a lot of iterations, think the wasm crate is in a much better place with these new patterns. Going to approve, but think there's one file where the helpers are small enough to move into the main function. After you do that, feel free to merge 👍

grod220 · 2023-09-19T19:15:53Z

crates/wasm/src/keys.rs

Think this file has some helper functions that are small enough to be moved into the main function. It's similar feedback as the other. Will have to move WasmResult to the the top level function.

Valentine1898 · 2023-09-20T16:59:38Z

@conorsch can merge this PR?

conorsch · 2023-09-20T19:03:27Z

There's just one more conflict, on the cargo lock file, that's blocking merge. I'll fix that now, and get it in!

Collects several improvements to the WASM interface for building transactions via the planner: * added wasm planner which allows more flexible creation of TransactionPlan on TS side * added logic of storing and reading advice when scanning blocks * added function for generating ephemeral address * planner reads data from indexedDB directly from rust Co-authored-by: Valentine <[email protected]>

conorsch · 2023-09-20T20:54:56Z

Turns out rebasing this PR on latest main was not straightforward. After an hour of unsuccessful interactive rebasing, I ended up performing a mixed reset on top of main, selectively adding only the wasm-related changes, and discarding the rest. That approach looks to have succeeded in preserving the new content of the PR while catching up the base with latest main.

In case I made a mistake, I made sure to push a "backup" branch to the penumbra remote: https://github.com/penumbra-zone/penumbra/tree/wasm_transaction_planner-backup That branch captures the state of this PR prior to my rebasing.

There's only one commit on this PR now, including the entirety of changes over the course of review. Tests and linters pass locally, so I think we're in good shape.

@grod220 and @Valentine1898: given your familiarity with the changes here, can you take one more look at the diff and confirm it conforms to expectations? If so, we're good to merge!

Valentine1898 · 2023-09-20T21:33:18Z

Oh, I didn't think merge would be a problem.
I reviewed the code again, it's all right.

grod220

@Valentine1898, please merge 🙏

Valentine1898 · 2023-09-21T09:29:17Z

I can't merge, I don't have permission to do that

grod220 · 2023-09-21T11:48:56Z

@Valentine1898 oh! Didn't realize. I feel like an approved PR by penumbra team member should enable external folks to merge 🤔 . I'll do it, all good.

zpoken had a problem deploying to smoke-test September 6, 2023 15:25 — with GitHub Actions Error

zpoken had a problem deploying to smoke-test September 6, 2023 15:53 — with GitHub Actions Error

zpoken had a problem deploying to smoke-test September 6, 2023 16:10 — with GitHub Actions Error

zpoken had a problem deploying to smoke-test September 6, 2023 16:19 — with GitHub Actions Error

zpoken had a problem deploying to smoke-test September 6, 2023 16:23 — with GitHub Actions Error

zpoken temporarily deployed to smoke-test September 6, 2023 16:32 — with GitHub Actions Inactive

grod220 reviewed Sep 6, 2023

View reviewed changes

zpoken temporarily deployed to smoke-test September 11, 2023 19:02 — with GitHub Actions Inactive

zpoken temporarily deployed to smoke-test September 12, 2023 00:32 — with GitHub Actions Inactive

zpoken had a problem deploying to smoke-test September 12, 2023 23:32 — with GitHub Actions Error

zpoken temporarily deployed to smoke-test September 12, 2023 23:38 — with GitHub Actions Inactive

zpoken temporarily deployed to smoke-test September 13, 2023 00:05 — with GitHub Actions Inactive

zpoken had a problem deploying to smoke-test September 13, 2023 15:27 — with GitHub Actions Error

zpoken temporarily deployed to smoke-test September 13, 2023 15:30 — with GitHub Actions Inactive

zpoken temporarily deployed to smoke-test September 15, 2023 00:22 — with GitHub Actions Inactive

zpoken had a problem deploying to smoke-test September 18, 2023 19:25 — with GitHub Actions Error

zpoken had a problem deploying to smoke-test September 18, 2023 19:30 — with GitHub Actions Error

zpoken temporarily deployed to smoke-test September 18, 2023 19:34 — with GitHub Actions Inactive

grod220 reviewed Sep 19, 2023

View reviewed changes

Valentine1898 reviewed Sep 19, 2023

View reviewed changes

zpoken had a problem deploying to smoke-test September 19, 2023 10:58 — with GitHub Actions Error

zpoken had a problem deploying to smoke-test September 19, 2023 11:07 — with GitHub Actions Error

zpoken had a problem deploying to smoke-test September 19, 2023 11:09 — with GitHub Actions Error

zpoken had a problem deploying to smoke-test September 19, 2023 11:15 — with GitHub Actions Error

zpoken had a problem deploying to smoke-test September 19, 2023 11:20 — with GitHub Actions Error

zpoken temporarily deployed to smoke-test September 19, 2023 11:30 — with GitHub Actions Inactive

grod220 reviewed Sep 19, 2023

View reviewed changes

zpoken temporarily deployed to smoke-test September 19, 2023 16:32 — with GitHub Actions Inactive

zpoken temporarily deployed to smoke-test September 19, 2023 18:14 — with GitHub Actions Inactive

grod220 approved these changes Sep 19, 2023

View reviewed changes

zpoken temporarily deployed to smoke-test September 19, 2023 19:47 — with GitHub Actions Inactive

conorsch force-pushed the wasm_transaction_planner branch from bf22635 to 9b83f2c Compare September 20, 2023 20:49

conorsch temporarily deployed to smoke-test September 20, 2023 20:49 — with GitHub Actions Inactive

conorsch self-requested a review September 20, 2023 20:55

conorsch approved these changes Sep 20, 2023

View reviewed changes

grod220 approved these changes Sep 21, 2023

View reviewed changes

grod220 merged commit cb57996 into penumbra-zone:main Sep 21, 2023

TalDerei mentioned this pull request Oct 29, 2023

Parallelize the building process using web-workers #3236

Closed


		/// Set the expiry height for the transaction plan.
		#[instrument(skip(self))]

		let tx = db.transaction_on_one("notes").ok().unwrap();
		let store = tx.object_store("notes").ok().unwrap();

Improvements and fixes to wasm transaction planner and wasm view server #2973

Improvements and fixes to wasm transaction planner and wasm view server #2973

Conversation

zpoken commented Sep 6, 2023

grod220 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

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 Sep 12, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conorsch commented Sep 8, 2023

grod220 commented Sep 12, 2023

Valentine1898 commented Sep 18, 2023

Valentine1898 commented Sep 18, 2023

grod220 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

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

Valentine1898 Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grod220 commented Sep 19, 2023

Valentine1898 commented Sep 19, 2023

Choose a reason for hiding this comment

Valentine1898 commented Sep 19, 2023 • edited Loading

grod220 Sep 19, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grod220 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Valentine1898 commented Sep 20, 2023

conorsch commented Sep 20, 2023

conorsch commented Sep 20, 2023

Valentine1898 commented Sep 20, 2023

grod220 left a comment • edited Loading

Choose a reason for hiding this comment

Valentine1898 commented Sep 21, 2023

grod220 commented Sep 21, 2023

Valentine1898 Sep 12, 2023 •

edited

Loading

Valentine1898 Sep 19, 2023 •

edited

Loading

Valentine1898 commented Sep 19, 2023 •

edited

Loading

grod220 Sep 19, 2023 •

edited

Loading

grod220 left a comment •

edited

Loading