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

Support backref for wasm #395

Merged
merged 12 commits into from
Apr 25, 2024
Merged

Support backref for wasm #395

merged 12 commits into from
Apr 25, 2024

Conversation

ChiaMineJP
Copy link
Contributor

@ChiaMineJP ChiaMineJP commented Apr 3, 2024

Added support compressed form of clvm byte representation.

Changed

  • Renamed wasm/src/api.rs to wasm/src/run_program.rs
  • Added allow_backrefs() method to Flag struct
  • run_clvm requires flag argument. If ALLOW_BACKREFS bit is set, it deserializes bytes input of clvm objects as compressed forms.
  • When ALLOW_BACKREFS bit is set when calling run_chia_program, it deserializes bytes input of clvm objects as compressed forms.

Added

  • Added sexp_from_bytes function.
  • Added to_bytes() method to LazyNode

@ChiaMineJP ChiaMineJP self-assigned this Apr 3, 2024
Copy link

coveralls-official bot commented Apr 3, 2024

Pull Request Test Coverage Report for Build 8829620756

Details

  • 29 of 79 (36.71%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.4%) to 93.947%

Changes Missing Coverage Covered Lines Changed/Added Lines %
wasm/src/serialize.rs 0 15 0.0%
wasm/src/lazy_node.rs 0 16 0.0%
wasm/src/run_program.rs 0 19 0.0%
Totals Coverage Status
Change from base Build 8310295107: -0.4%
Covered Lines: 5789
Relevant Lines: 6162

💛 - Coveralls

wasm/src/serialize.rs Outdated Show resolved Hide resolved
wasm/src/serialize.rs Outdated Show resolved Hide resolved
wasm/src/lazy_node.rs Outdated Show resolved Hide resolved
wasm/src/lazy_node.rs Outdated Show resolved Hide resolved
src/flags.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@richardkiss richardkiss left a comment

Choose a reason for hiding this comment

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

Consider moving ALLOW_BACKREFS to wasm api, or simply exporting node_to_bytes_backrefs separately.

Co-authored-by: Arvid Norberg <[email protected]>
@ChiaMineJP
Copy link
Contributor Author

ChiaMineJP commented Apr 4, 2024

Consider moving ALLOW_BACKREFS to wasm api, or simply exporting node_to_bytes_backrefs separately.

I'll move ALLOW_BACKREFS to wasm api.
In Rust part of clvm_rs, whether to allow backrefs will be decided in the upper scope (like chia_rs).
However, in wasm/js part of clvm_rs, developers must tell whether to allow backref or not in the clvm_wasm(clvm_rs)'s scope because from JS world, SExp is always passed as uint8 byte array.
So whether the byte stream is compressed or not must be immediately known in run_chia_program or run_clvm and cannot defer.

wasm/src/lazy_node.rs Outdated Show resolved Hide resolved
src/serde/ser_br.rs Show resolved Hide resolved
@ChiaMineJP ChiaMineJP requested a review from arvidn April 10, 2024 15:18
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

my main comment is that I don't think node_to_bytes_backrefs() should have a default size limit

src/serde/ser_br.rs Outdated Show resolved Hide resolved
wasm/src/serialize.rs Outdated Show resolved Hide resolved
arvidn
arvidn previously approved these changes Apr 16, 2024
richardkiss
richardkiss previously approved these changes Apr 19, 2024
@ChiaMineJP ChiaMineJP dismissed stale reviews from richardkiss and arvidn via d55079c April 24, 2024 10:38
@ChiaMineJP ChiaMineJP requested a review from arvidn April 24, 2024 10:38
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

looks good!

wasm/src/serialize.rs Show resolved Hide resolved
@arvidn arvidn closed this Apr 25, 2024
@arvidn arvidn reopened this Apr 25, 2024
@ChiaMineJP ChiaMineJP merged commit d6db84d into main Apr 25, 2024
46 of 48 checks passed
@ChiaMineJP ChiaMineJP deleted the cmj.backref-for-wasm branch April 25, 2024 15:13
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.

3 participants