Optimize check owned by removing json and fix double rkyv serialization bug

[Daksh14]

Closes #113 and #114

This is the wasm binary that is in the latest dusk-wallet-js version, we should merge into main ASAP

[ZER0]

Overall looks good, but there are few nits to change, one very important tied to performance and scalar multiplication.

[ZER0]

Still few things to change, but we're on the right track. Be sure to do not derive the keys inside any loops, but doing so upfront. If we notice this as common pattern, (the need of sk and vk together), we might also have an inline function that does that for us, so we don't have to repeat it.

[ZER0]

You don't need to wrap the MAX_KEY in parentheses, the previous code would works fine.

[ZER0]

There is the part related to the loops and labels usage (see the comment). Once it's fixed I will approve immediately (that loop and others like that).

I would suggest to fix it in this PR to avoid open a separate issue, but if you're more comfortable to file a new issue and fix it there immediately please file the issue and add its link as reply to the conversation in this review concerning the matter; I'm also fine with that as soon as it's fixed ASAP.

[ZER0]

You still didn't modify this loop as discussed.

1. The comment is wrong, or best case scenario misleading
2. The usage of labels makes the flow harder to follow from top to bottom – it's "GOTO-like" –; therefore the labels should be used only when there is not any decent alternative available
3. Due to the label usage, the code makes use of a temp var called keys_len that shouldn't be there.

All those should be fixed, if you don't want to do it in this PR feel free to open immediately a new one – but also an issue related to that – to apply those changes. I suggested to do it in this PR since you're already touching this code and in addition you can skip to create a separate issue. But if you prefer to file a new issue and fix it immediately in a new PR, it's also fine.

My suggestion, as I already mentioned to you, would be modify this using a more functional approach with try_fold. Here is the code:

    let nullifiers = notes.iter().try_fold(
        Vec::with_capacity(notes.len()),
        |mut nullifiers, note| {
            if let Some(idx) = (0..MAX_KEY).find(|&i| vks[i].owns(note)) {
                let nullifier = note.gen_nullifier(&sks[idx]);
                nullifiers.push(nullifier);
                Ok(nullifiers)
            } else {
                Err(())
            }
        },
    );

    match nullifiers {
        Ok(n) => utils::rkyv_into_ptr(n),
        Err(_) => utils::fail(),
    }

Basically, if you cannot decrypt a note with any of the keys you own, than the function calls utils::fail.

[Daksh14]

I removed the comment and the label, I think any greater refractor than that would be counter productive since the "big" refractor is imminent, I have already created the EPIC for refractor I'll add this content to it
#117

[ZER0]

LGTM, just few minor nits before merging. See the comments, but to summarize:

1. rename vk_idx into idx
2. remove the unused core::mem as stated by github too.
3. remove the sorrounding parentheses from MAX_KEY from tests.

[ZER0]

I forgot about position, this is much better. Just call it idx or i.