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.

Sign up for GitHub

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

Jump to bottom

refactor(wasm): modularize wasm crate logic #3432

Merged
merged 10 commits into from
Nov 28, 2023
Merged

refactor(wasm): modularize wasm crate logic #3432

merged 10 commits into from
Nov 28, 2023

Conversation

TalDerei
Copy link
Collaborator

@TalDerei TalDerei commented Nov 25, 2023 •
edited
Loading

References #3431 and #3400.

@TalDerei TalDerei self-assigned this Nov 25, 2023
@TalDerei TalDerei requested a review from grod220 November 25, 2023 04:39
@TalDerei TalDerei mentioned this pull request Nov 25, 2023
Closed
@TalDerei TalDerei marked this pull request as ready for review November 25, 2023 04:47
@TalDerei
Copy link
Collaborator Author

TalDerei commented Nov 25, 2023 •
edited
Loading

@hdevalence I'm realizing build_action doesn't need the transaction_plan field in the method signature. It's deserializing an entire TransactionPlan just to access the memo_plan which can simply be passed in from the web-side.

@hdevalence
Copy link
Member

Yeah, I thought the reason for that was that it would be simpler to only pass one object over FFI into wasm. Even if it's a little inefficient, that's fine if the code becomes simpler, as long as it's not the dominant performance factor.

grod220
grod220 reviewed Nov 27, 2023
View reviewed changes
Copy link
Contributor

@grod220 grod220 left a comment

Choose a reason for hiding this comment

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

For the wasm-compilation script, left comments in the other PR 👍

crates/wasm/src/build.rs Outdated Show resolved Hide resolved
crates/wasm/src/build.rs Outdated Show resolved Hide resolved
crates/wasm/src/build.rs Outdated Show resolved Hide resolved
crates/wasm/src/wasm_planner.rs Outdated Show resolved Hide resolved
@TalDerei TalDerei requested a review from grod220 November 28, 2023 06:19
@TalDerei
Copy link
Collaborator Author

This PR is blocked until #3429 is merged.

@TalDerei TalDerei merged commit f08d72d into main Nov 28, 2023
6 checks passed
@TalDerei TalDerei deleted the wasm-builder branch November 28, 2023 20:44
conorsch added a commit that referenced this pull request Nov 29, 2023
@conorsch
build: remove unused wasm profile
805ede1 
Follow-up to #3432. Shutting this customization off, because it was
being ignored anyway, and generated a warnig:

    warning: profiles for the non root package will be ignored,
    specify profiles at the workspace root

We should discuss whether keeping the wasm crate inside the workspace is
the right move.
@conorsch conorsch mentioned this pull request Nov 29, 2023
Merged
conorsch added a commit that referenced this pull request Nov 29, 2023
@conorsch
build: remove unused wasm profile
7a82b53 
Follow-up to #3432. Shutting this customization off, because it was
being ignored anyway, and generated a warnig:

    warning: profiles for the non root package will be ignored,
    specify profiles at the workspace root

We should discuss whether keeping the wasm crate inside the workspace is
the right move.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hdevalence hdevalence hdevalence left review comments

@grod220 grod220 grod220 approved these changes

Assignees

@TalDerei TalDerei

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TalDerei @hdevalence @grod220