-
Notifications
You must be signed in to change notification settings - Fork 17
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
Generate unbonding token metadata on the fly for the approval dialog #896
Conversation
eb08320
to
23d0ef1
Compare
const existing0thEpoch = await instance.getEpochByHeight(0n); | ||
if (!existing0thEpoch) await instance.addEpoch(0n); // Create first epoch |
There was a problem hiding this comment.
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.
@@ -1,6 +1,6 @@ | |||
|
|||
PRAX=lkpmkhpnhknhmibgnmmhdhgdilepfghe | |||
IDB_VERSION=32 | |||
IDB_VERSION=33 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed for this fix
return denomMetadata ?? new Metadata({ penumbraAssetId: assetId }); | ||
} catch { | ||
return new Metadata(); | ||
return new Metadata({ penumbraAssetId: assetId }); |
There was a problem hiding this comment.
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.
packages/wasm/crate/src/metadata.rs
Outdated
"^udelegation_(?P<data>penumbravalid1[a-zA-HJ-NP-Z0-9]+)$"; | ||
pub static SHORTENED_ID_LENGTH: usize = 8; | ||
|
||
pub fn customize_symbol(metadata: Metadata) -> Metadata { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
andunwrap
(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: ®ex::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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions to improve customize_symbol
. After that 👍
packages/wasm/crate/src/metadata.rs
Outdated
"^udelegation_(?P<data>penumbravalid1[a-zA-HJ-NP-Z0-9]+)$"; | ||
pub static SHORTENED_ID_LENGTH: usize = 8; | ||
|
||
pub fn customize_symbol(metadata: Metadata) -> Metadata { |
There was a problem hiding this comment.
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?
andunwrap
(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: ®ex::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.
packages/wasm/crate/src/planner.rs
Outdated
let metadata_proto = metadata.to_proto(); | ||
let customized_metadata_proto = customize_symbol(metadata_proto); | ||
let customized_metadata = Metadata::try_from(customized_metadata_proto)?; |
There was a problem hiding this comment.
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?
When a user undelegates from a validator — meaning, they want to unbond their delegation tokens so that they can reclaim those tokens as UM — Penumbra issues "unbonding" tokens. These unbonding tokens are tied to the specific validator that the user is undelegating from, as well as to the block height at which the user made the undelegate transaction. (It just so happens that Penumbra's current implementation clamps the block height to the start of the current epoch; but this could change in the future, so we treat it as though it's tied to a block height, rather than tied to an epoch.)
Because unbonding tokens are so specific, there often is no metadata for them in the database when a user wishes to undelegate from a validator. (The only way there would be metadata for them in the database is if the user had already undelegated from that same validator at that same block height at least once before. Since the block height is clamped to the start of the epoch, this is technically possible; but the user would still face this issue the first time they undelegate from that validator during that epoch.) As a result, the transaction approval dialog would show "Unknown asset" for the "Output" action that outputs unbonding tokens. This is a confusing experience for users, who would have no way of knowing that the "Unknown asset" they're getting is the unbonding token.
This PR fixes that issue by generating and saving asset metadata to the database while planning the transaction. This means that, by the time the transaction approval dialog displays the transaction plan, the metadata will exist in the database, and thus the dialog will correctly show the asset metadata.
The (very slight) downside to this approach is that, if the user cancels the transaction and never attempts another undelegation at that block height, IndexedDB will contain metadata for an asset that doesn't actually exist for the user on-chain. i.e., if the user clicks "Undelegate" for a validator — which pops open the transaction approval dialog and saves the generated asset metadata for the unbonding tokens — and then cancels the transaction, the generated metadata will still exist in the database even if it never gets "used" for any unbonding tokens. This seems like an OK trade-off, however.
Closes #856