Add tx decode to js value to wasm lib #1956
Conversation
| console.log("decoded tx format:"); | ||
| console.dir(decoded_tx, { depth: null }); | ||
|
|
||
| // top-level check | ||
| if (!decoded_tx.V1) { | ||
| throw new Error("Missing V1 field in decoded transaction"); | ||
| } |
There was a problem hiding this comment.
This test is very verbose but also very limited. Why not specify the entire expected JS object and then, e.g., compare their JSON representation: JSON.stringify(decoded_tx) === JSON.stringify(expected_decoded_tx)?
And console.dir(decoded_tx, { depth: null }) can be removed, it pollutes the tests' output.
wasm-wrappers/src/lib.rs
Outdated
| let value = | ||
| serde_wasm_bindgen::to_value(tx.transaction()).expect("Should not fail to create js value"); | ||
| Ok(value) | ||
| } |
There was a problem hiding this comment.
- Why only
tx.transaction()and not the entire tx? Why not show the signatures? - In the output we'll now have things like "HexifiedDelegationId" and "HexifiedDestination". Why not dehexify them before returning? We already have functions
dehexify_all_addressesandto_dehexified_json. The latter returnsserde_json::Valueand I thinkserde_wasm_bindgen::to_valueshould be able to accept it.
OBorce
force-pushed
the
feature/wasm-tx-decode
branch
2 times, most recently
from
40e16ee to
f1feb95
Compare
| if (decoded_tx !== JSON.stringify(expected_decoded_tx)) { | ||
| throw new Error("Decoded transaction does not match expected"); | ||
| } |
There was a problem hiding this comment.
Use assert_eq_vals instead? It will print both values on mismatch, making it easier to debug in case of future breaking changes.
wasm-wrappers/src/lib.rs
Outdated
| @@ -689,6 +691,23 @@ pub fn encode_transaction(inputs: &[u8], outputs: &[u8], flags: u64) -> Result<V | |||
| Ok(tx.encode()) | |||
| } | |||
|
|
|||
| /// Decodes a signed transaction from its binary encoding into a Json string. | |||
| #[wasm_bindgen] | |||
| pub fn decode_signed_transaction_to_json_str( | |||
There was a problem hiding this comment.
Why did you change it to return a string? IMO it's very strange from the API point of view.
Also, returning JsValue now makes no sense, it only makes the generated TS function signature confusing (it says it returns any, though in reality it returns only string).
There was a problem hiding this comment.
Producing a JsValue from a serde Json value gives a JS Map and not an object as before. Map cannot be put through Json stringify for comparison in the test and accessing fields needs to go through the get() method...
With a string you can always do a Json parse and get an object. Not sure what is the best tradeoff.
There was a problem hiding this comment.
Producing a JsValue from a serde Json value gives a JS Map and not an object as before. Map cannot be put through Json stringify for comparison in the test and accessing fields needs to go through the get() method... With a string you can always do a Json parse and get an object. Not sure what is the best tradeoff.
I've just checked and js_sys::JSON::parse seems to do the trick, i.e. it produces an object and not a map.
P.S. js_sys::JSON::parse returns Result<JsValue, JsValue>, let's not expect on it and define an Error variant for its result instead. The reason is (besides expects being bad in general) that it's a complex call, where multiple things could go wrong (as I understand, it calls back into JS to produce a JS object and then returns a handle to it).
OBorce
force-pushed
the
feature/wasm-tx-decode
branch
from
f1feb95 to
98a00b1
Compare
wasm-wrappers/src/error.rs
Outdated
| #[error("Invalid signed transaction encoding: {0}")] | ||
| InvalidSignedTransactionEncoding(serialization::Error), | ||
|
|
There was a problem hiding this comment.
Btw, I don't think we need this. E.g. in extract_htlc_secret we return InvalidTransactionEncoding when decoding SignedTransaction. Also, we return TransactionCreationError on SignedTransaction::new failures. I.e. we don't differentiate between the ordinary and signed transactions when it comes to errors.
wasm-wrappers/src/lib.rs
Outdated
| @@ -689,6 +691,23 @@ pub fn encode_transaction(inputs: &[u8], outputs: &[u8], flags: u64) -> Result<V | |||
| Ok(tx.encode()) | |||
| } | |||
|
|
|||
| /// Decodes a signed transaction from its binary encoding into a Json string. | |||
| #[wasm_bindgen] | |||
| pub fn decode_signed_transaction_to_json_str( | |||
There was a problem hiding this comment.
Producing a JsValue from a serde Json value gives a JS Map and not an object as before. Map cannot be put through Json stringify for comparison in the test and accessing fields needs to go through the get() method... With a string you can always do a Json parse and get an object. Not sure what is the best tradeoff.
I've just checked and js_sys::JSON::parse seems to do the trick, i.e. it produces an object and not a map.
P.S. js_sys::JSON::parse returns Result<JsValue, JsValue>, let's not expect on it and define an Error variant for its result instead. The reason is (besides expects being bad in general) that it's a complex call, where multiple things could go wrong (as I understand, it calls back into JS to produce a JS object and then returns a handle to it).
OBorce
force-pushed
the
feature/wasm-tx-decode
branch
from
1bc3801 to
79b18fd
Compare
OBorce
force-pushed
the
feature/wasm-tx-decode
branch
from
79b18fd to
5fcbd34
Compare
OBorce
force-pushed
the
feature/wasm-tx-decode
branch
from
5fcbd34 to
f589573
Compare
Add a new function to wasm lib to decode a signed transaction into a js value