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

Clarify wasm func naming #833

Merged
merged 2 commits into from
Mar 27, 2024
Merged

Clarify wasm func naming #833

merged 2 commits into from
Mar 27, 2024

Conversation

grod220
Copy link
Collaborator

@grod220 grod220 commented Mar 26, 2024

Follow up from: https://github.com/penumbra-zone/web/pull/821/files#r1538165300

A small PR to improve the naming of the wasm function getting index from an address.

@grod220 grod220 requested a review from jessepinho March 26, 2024 13:38
@Valentine1898 Valentine1898 self-requested a review March 27, 2024 10:11
Copy link
Contributor

@Valentine1898 Valentine1898 left a comment

Choose a reason for hiding this comment

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

LGTM

/// Arguments:
/// full_viewing_key: `bech32 String`
/// address: `bech32 String`
/// Returns: `Option<pb::AddressIndex>`
#[wasm_bindgen]
pub fn is_controlled_address(full_viewing_key: &str, address: &str) -> WasmResult<JsValue> {
pub fn get_index_by_address(full_viewing_key: &str, address: &str) -> WasmResult<JsValue> {
Copy link
Contributor

Choose a reason for hiding this comment

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

We pass full_viewing_key and address here as bech32 strings, although we can use the proto types FullViewingKey and Address.
In addition, we manually convert Address to bech32

get_index_by_address(fullViewingKey, bech32Address(address)) as JsonValue;

But at the same time, fvk always exists in extension as a bech32 string

I don't think we should fix it in this PR since it doesn't only affect the get_index_by_address function but the whole wasm, but I wonder what you think about it .

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we manually convert Address to bech32

Ah, I should fix this in this PR 👍

fvk always exists in extension as a bech32 string

Indeed, it is a bit strange we pass fvk around as a string versus use the bufbuild type for this. We should definitely update that before mainnet, can you make a ticket for this?

@grod220 grod220 force-pushed the controlled-naming branch from d06562c to 94e7b5e Compare March 27, 2024 12:03
@grod220 grod220 force-pushed the controlled-naming branch from 94e7b5e to 64a3d89 Compare March 27, 2024 12:14
@grod220 grod220 merged commit eafdce0 into main Mar 27, 2024
6 checks passed
@grod220 grod220 deleted the controlled-naming branch March 27, 2024 12:22
@grod220 grod220 self-assigned this Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants