0xMiden / 0xMiden/node

Follow-ups of `protocol` 0.15 integration

Open
#2,135 1 comment 1 reaction 0 assignees View on GitHub
Dominant language
Rust
Stars
104
Forks
138
Avg merge
1d 13h
Merged PRs (30d)
56

Description

#2095 had various comments after upgrading the protocol to version 0.15. https://github.com/0xMiden/node/pull/2132 addresses all of them, except for these remaining ones:

- `conv.rs` dedup: de-duplicate the conversion helpers shared between `conv.rs` and `conv.rs`, and rename the module to `conversions.rs`. ([link](https://github.com/0xMiden/node/pull/2095#discussion_r3293878039))
- ~~`details_commitment` design question: do we still need `details_commitment`, or can we drop the column? Investigate how we use it and potentially remove it. ([link](https://github.com/0xMiden/node/pull/2095#discussion_r3292382546))~~ <- ended up being addressed in #2132
- ~~`get_account()` is expensive: `get_account()` deserializes the full `Account` on every partial delta; switch to applying updates instead of loading the whole account each time. ([link](https://github.com/0xMiden/node/pull/2095#discussion_r3293972017))~~ tracked in #2363
- ~~`helpers.rs` explanation: add comments explaining why these helpers live here instead of upstream. ([link](https://github.com/0xMiden/node/pull/2095#discussion_r3293951779))~~ no longer relevant, code has move to crypto.
- `Felt::new_unchecked` safety: `Felt::new_unchecked` / `Word::unchecked` are used with `Rng`. Already tracked upstream in `crypto#1025`. [link](https://github.com/0xMiden/node/pull/2095#discussion_r3288189838)
Related upstream tracking issue: https://github.com/0xMiden/miden-vm/issues/3531

Some of these may require their own issue/discussion, but for now I'm writing them here to have the backlog up to date

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.