Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate unbonding token metadata on the fly for the approval dialog #896

Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/extension/.env
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@

PRAX=lkpmkhpnhknhmibgnmmhdhgdilepfghe
IDB_VERSION=32
IDB_VERSION=33
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed for this fix

USDC_ASSET_ID="reum7wQmk/owgvGMWMZn/6RFPV24zIKq3W6In/WwZgg="
MINIFRONT_URL=https://app.testnet.penumbra.zone/
PENUMBRA_NODE_PD_URL=https://grpc.testnet.penumbra.zone/
4 changes: 2 additions & 2 deletions apps/extension/src/state/tx-approval.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ export const createTxApprovalSlice = (): SliceCreator<TxApprovalSlice> => (set,
const getMetadata = async (assetId: AssetId) => {
try {
const { denomMetadata } = await viewClient.assetMetadataById({ assetId });
return denomMetadata ?? new Metadata();
return denomMetadata ?? new Metadata({ penumbraAssetId: assetId });
} catch {
return new Metadata();
return new Metadata({ penumbraAssetId: assetId });
Comment on lines +69 to +71
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly needed for this PR, but it should have been like this already.

}
};

Expand Down
5 changes: 4 additions & 1 deletion packages/storage/src/indexed-db/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,10 @@ export class IndexedDb implements IndexedDbInterface {

const instance = new this(db, new IbdUpdater(db), constants, chainId);
await instance.saveLocalAssetsMetadata(); // Pre-load asset metadata
await instance.addEpoch(0n); // Create first epoch

const existing0thEpoch = await instance.getEpochByHeight(0n);
if (!existing0thEpoch) await instance.addEpoch(0n); // Create first epoch
Comment on lines +122 to +123
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't realized that this code was run every time the extension was restarted, so I added a check to make sure the 0th epoch doesn't exist before creating it.


return instance;
}
close(): void {
Expand Down
1 change: 1 addition & 0 deletions packages/types/src/indexed-db.ts
Original file line number Diff line number Diff line change
Expand Up @@ -219,4 +219,5 @@ export const IDB_TABLES: Tables = {
prices: 'PRICES',
validator_infos: 'VALIDATOR_INFOS',
transactions: 'TRANSACTIONS',
full_sync_height: 'FULL_SYNC_HEIGHT',
};
1 change: 1 addition & 0 deletions packages/wasm/crate/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/wasm/crate/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ hex = "0.4.3"
indexed_db_futures = "0.4.1"
prost = "0.12.3"
rand_core = { version = "0.6.4", features = ["getrandom"] }
regex = { version = "1.8.1" }
serde = { version = "1.0.197", features = ["derive"] }
serde-wasm-bindgen = "0.6.5"
thiserror = "1.0"
Expand Down
1 change: 1 addition & 0 deletions packages/wasm/crate/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ pub mod build;
pub mod dex;
pub mod error;
pub mod keys;
pub mod metadata;
pub mod note_record;
pub mod planner;
pub mod storage;
Expand Down
42 changes: 42 additions & 0 deletions packages/wasm/crate/src/metadata.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
use penumbra_proto::core::asset::v1::Metadata;
use regex::Regex;

pub static UNBONDING_TOKEN_REGEX: &str = "^uunbonding_(?P<data>start_at_(?P<start>[0-9]+)_(?P<validator>penumbravalid1(?P<id>[a-zA-HJ-NP-Z0-9]+)))$";
pub static DELEGATION_TOKEN_REGEX: &str =
"^udelegation_(?P<data>penumbravalid1[a-zA-HJ-NP-Z0-9]+)$";
jessepinho marked this conversation as resolved.
Show resolved Hide resolved
pub static SHORTENED_ID_LENGTH: usize = 8;

pub fn customize_symbol(metadata: Metadata) -> Metadata {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the exact same functionality as our TypeScript customizeSymbol function, ported to Rust.

I briefly spiked on just calling out to this Rust function from our TypeScript code, rather than having one copy of this function on each of the Rust and TypeScript sides. However, that would mean modifying this function to take a Vec<u8> encoding of the metadata object, which would then make this function a bit messier to use from the Rust planner code that calls it, because we'd have to encode and decode metadata to achieve this.

Not sure what the best way forward for this is — reviewers, thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

  • In Rust, save for tests, we should try to avoid panic'ing. A panic is basically like a hard crash where we couldn't even catch on the javascript side. Which means.expect(), should be replaced with ? and unwrap (unless guaranteed to never panic) should be handled.
  • To expose this function to the frontend, perhaps we can have an _inner function? Whatever naming, we can have two functions, one to handle TS and one for internal wasm usage.

What do you think of this?

// along with other errors in error.rs...
#[derive(Error, Debug)]
pub enum WasmError {
    // ...
    #[error("{0}")]
    RegexError(#[from] regex::Error),
}

#[wasm_bindgen]
pub fn customize_symbol(metadata_bytes: &[u8]) -> WasmResult<Vec<u8>> {
    utils::set_panic_hook();

    let metadata = Metadata::decode(metadata_bytes)?;
    customize_symbol_inner(metadata)?;
    Ok(metadata.encode_to_vec())
}


pub fn customize_symbol_inner(mut metadata: Metadata) -> WasmResult<Metadata> {
    let unbonding_re = Regex::new(UNBONDING_TOKEN_REGEX)?;
    let delegation_re = Regex::new(DELEGATION_TOKEN_REGEX)?;

    if let Some(captures) = unbonding_re.captures(&metadata.base) {
        let shortened_id = shorten_id(&captures)?;
        let start_match = captures.name("start")
            .ok_or_else(|| anyhow!("<start> not matched in unbonding token regex"))?
            .as_str();
        metadata.symbol = format!("unbondUMat{start_match}({shortened_id}...)");
    } else if let Some(captures) = delegation_re.captures(&metadata.base) {
        let shortened_id = shorten_id(&captures)?;
        metadata.symbol = format!("delUM({shortened_id}...)");
    }

    Ok(metadata)
}

fn shorten_id(captures: &regex::Captures) -> WasmResult<String> {
    let id_match = captures.name("id").ok_or_else(|| anyhow!("<id> not matched in staking token regex"))?;
    Ok(id_match.as_str()
        .chars()
        .take(SHORTENED_ID_LENGTH)
        .collect())
}

If you don't like mutating the input, this can be converted to just cloning the input and mutating that.

if let Some(unbonding_match) = Regex::new(UNBONDING_TOKEN_REGEX)
.expect("regex is valid")
.captures(&metadata.base)
{
let id_match = unbonding_match.name("id").unwrap().as_str();
let shortened_id = id_match
.chars()
.take(SHORTENED_ID_LENGTH)
.collect::<String>();
let start_match = unbonding_match.name("start").unwrap().as_str();

Metadata {
symbol: format!("unbondUMat{start_match}({shortened_id}…)"),
..metadata
}
} else if let Some(delegation_match) = Regex::new(DELEGATION_TOKEN_REGEX)
.expect("regex is valid")
.captures(&metadata.base)
{
let id_match = delegation_match.name("id").unwrap().as_str();
let shortened_id = id_match
.chars()
.take(SHORTENED_ID_LENGTH)
.collect::<String>();

Metadata {
symbol: format!("delUM({shortened_id}…)"),
..metadata
}
} else {
metadata
}
}
37 changes: 31 additions & 6 deletions packages/wasm/crate/src/planner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::collections::BTreeMap;
use anyhow::anyhow;
use ark_ff::UniformRand;
use decaf377::{Fq, Fr};
use penumbra_asset::asset::Metadata;
use penumbra_asset::{asset, Balance, Value};
use penumbra_dex::swap_claim::SwapClaimPlan;
use penumbra_dex::{
Expand All @@ -22,7 +23,7 @@ use penumbra_proto::DomainType;
use penumbra_sct::params::SctParameters;
use penumbra_shielded_pool::{fmd, OutputPlan, SpendPlan};
use penumbra_stake::rate::RateData;
use penumbra_stake::{IdentityKey, Penalty, UndelegateClaimPlan};
use penumbra_stake::{IdentityKey, Penalty, Undelegate, UndelegateClaimPlan};
use penumbra_transaction::gas::GasCost;
use penumbra_transaction::memo::MemoPlaintext;
use penumbra_transaction::{plan::MemoPlan, ActionPlan, TransactionParameters, TransactionPlan};
Expand All @@ -31,6 +32,7 @@ use rand_core::OsRng;
use wasm_bindgen::prelude::wasm_bindgen;
use wasm_bindgen::JsValue;

use crate::metadata::customize_symbol;
use crate::note_record::SpendableNoteRecord;
use crate::storage::IndexedDBStorage;
use crate::utils;
Expand Down Expand Up @@ -319,11 +321,12 @@ pub async fn plan_transaction(
let rate_data: RateData = rate_data
.ok_or_else(|| anyhow!("missing rate data in undelegation"))?
.try_into()?;
actions.push(
rate_data
.build_undelegate(epoch.into(), value.amount)
.into(),
);

let undelegate = rate_data.build_undelegate(epoch.into(), value.amount);

save_unbonding_token_metadata_if_needed(&undelegate, &storage).await?;

actions.push(undelegate.into());
}

for tpr::UndelegateClaim {
Expand Down Expand Up @@ -480,3 +483,25 @@ pub async fn plan_transaction(

Ok(serde_wasm_bindgen::to_value(&plan)?)
}

/// When planning an undelegate action, there may not be metadata yet in the
/// IndexedDB database for the unbonding token that the transaction will output.
/// That's because unbonding tokens are tied to a specific height. If unbonding
/// tokens for a given validator and a given height don't exist yet, we'll
/// generate them here and save them to the database, so that they can render
/// correctly in the transaction approval dialog.
async fn save_unbonding_token_metadata_if_needed(
undelegate: &Undelegate,
storage: &IndexedDBStorage,
) -> WasmResult<()> {
let metadata = undelegate.unbonding_token().denom();

if storage.get_asset(&metadata.id()).await?.is_none() {
let metadata_proto = metadata.to_proto();
let customized_metadata_proto = customize_symbol(metadata_proto);
let customized_metadata = Metadata::try_from(customized_metadata_proto)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we can't access metadata fields so we convert to proto and back to do so? Hmm. Is this a pattern in the core repo too?

storage.add_asset(&customized_metadata).await
} else {
Ok(())
}
}
26 changes: 26 additions & 0 deletions packages/wasm/crate/src/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ pub struct Tables {
pub gas_prices: String,
pub epochs: String,
pub transactions: String,
pub full_sync_height: String,
}

pub struct IndexedDBStorage {
Expand Down Expand Up @@ -176,6 +177,31 @@ impl IndexedDBStorage {
.transpose()?)
}

pub async fn add_asset(&self, metadata: &Metadata) -> WasmResult<()> {
let tx = self
.db
.transaction_on_one_with_mode(&self.constants.tables.assets, Readwrite)?;
let store = tx.object_store(&self.constants.tables.assets)?;
let metadata_js = serde_wasm_bindgen::to_value(&metadata.to_proto())?;

store.put_val_owned(&metadata_js)?;

Ok(())
}

pub async fn get_full_sync_height(&self) -> WasmResult<Option<u64>> {
let tx = self
.db
.transaction_on_one(&self.constants.tables.full_sync_height)?;
let store = tx.object_store(&self.constants.tables.full_sync_height)?;

Ok(store
.get_owned("height")?
.await?
.map(serde_wasm_bindgen::from_value)
.transpose()?)
}

pub async fn get_note(
&self,
commitment: &note::StateCommitment,
Expand Down
2 changes: 2 additions & 0 deletions packages/wasm/crate/tests/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ mod tests {
gas_prices: String,
epochs: String,
transactions: String,
full_sync_height: String,
}

// Define `IndexDB` table parameters and constants.
Expand All @@ -99,6 +100,7 @@ mod tests {
gas_prices: "GAS_PRICES".to_string(),
epochs: "EPOCHS".to_string(),
transactions: "TRANSACTIONS".to_string(),
full_sync_height: "FULL_SYNC_HEIGHT".to_string(),
};

let constants: IndexedDbConstants = IndexedDbConstants {
Expand Down
Loading