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

feat: add address validations #96

Merged
merged 3 commits into from
Aug 11, 2022

Conversation

joacohoyos
Copy link
Contributor

Resolves #95

Address 3 extra functions to avoid wasm-bindgen memory leak ](rustwasm/wasm-bindgen#1963).

is_shelley -> validates if the string is a valid shelley address
is_byron -> validates if the string is a valid byron address
is_valid -> validates if the string is valid address both shelley or byron

@SebastienGllmt
Copy link
Contributor

Is this still relevant? I thought the upstream stack leak issue was fixed. Honestly I haven't looked into it since it's been years since I originally reported the stack leak to the wasm-bindgen team

@joacohoyos
Copy link
Contributor Author

joacohoyos commented Aug 3, 2022

@SebastienGllmt I'm currently getting the memory leak error both with cardano-multiplatform-lib and with @emurgo/cardano-serialization-lib

@joacohoyos
Copy link
Contributor Author

@SebastienGllmt Also desptie the stack leak issue, which seems to still be happening, I think that having the 3 functions would be a nice feature, this way we can have a simple boolean function to know if an address is valid

@SebastienGllmt
Copy link
Contributor

SebastienGllmt commented Aug 3, 2022

Ah, I looked back into it and now I remember what happened. They finally fixed the issue and then didn't make a release that included it for months so I gave up hope. It look like version 0.2.80 of wasm-bindgen includes the fix.

Can you try bumping to 0.2.80 (or later) to see if this solves the issue for you?

@joacohoyos
Copy link
Contributor Author

joacohoyos commented Aug 3, 2022

@SebastienGllmt I tried bumping the version but now I'm getting the following error, I'm not quite sure how to fix it as apparently there is a JsError implemented in src/errors. Sorry if I'm making any basic question but it's literally my first time writting rust

function or associated item not found in `wasm_bindgen::JsError`

image

@SebastienGllmt
Copy link
Contributor

hmm, seems they made a breaking change in bindgen for the error type. Will have to look into it

@joacohoyos
Copy link
Contributor Author

Okk great! If you think the function would be a good add feel free to merge it. Personally I believe there are some cases were it will be useful to have this kind of functions

@rooooooooob
Copy link
Contributor

rooooooooob commented Aug 4, 2022

I think this was actually an issue half caused by the upgrade in bindgen/js-sys and half caused by #81 which moved some stuff calling JsError around which was a typedef to JsValue on wasm builds (which it probably should have been JsError to begin with for js usage - likely this was a relic from basing code off of js-chain-libs that used JsValue ) and its own JsError type for non-wasm builds to avoid panics.

So after refactoring utils.rs part of the code was now using js_sys::JsError directly instead of finding the type JsError = js_sys::JsValue typedef, and then I guess they removed the from_str() method of JsError.

Since only some places had

use crate::error::JsError;

so only some would use the definitions found at:

#[cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))]
pub type JsError = JsValue;

#[cfg(not(all(target_arch = "wasm32", not(target_os = "emscripten"))))]
#[derive(Debug, Clone)]
pub struct JsError {
    msg: String,
}

and others would use JsError instead of JsValue due to the wildcard wasm_bindgen import in lib.rs.

I wanted to just blanket switch these all to JsError, however one problem is that js_sys::JsError does not implement Debug which breaks a lot of existing code. In the future we should use custom error types as those seem to be allowed as of rustwasm/wasm-bindgen#2710

Expands the acceptable return-position types to Result<T, E> where T: IntoWasmAbi, E: Into, so you can easily throw your own custom error classes imported from JS

@joacohoyos

Here's a PR (untested besides compiling it in rust+wasm/running rust tests) that updates with this fix so try that out: #98

@joacohoyos
Copy link
Contributor Author

joacohoyos commented Aug 5, 2022

@rooooooooob Thanks! I just checked it seems to have fixed the memory leak error. I left a small comment regarding an error on build but we can keep that discussion in the other PR.

joacohoyos and others added 2 commits August 11, 2022 09:34
…dations

# Conflicts:
#	rust/pkg/cardano_multiplatform_lib.js.flow
@SebastienGllmt
Copy link
Contributor

I slightly changed the API since unfortunately address encoding is a bit tricker than you may think. Should be good-to-go now

@SebastienGllmt SebastienGllmt merged commit 715af08 into dcSpark:develop Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add is_valid function to Address
3 participants