Closed ross-weir closed 1 year ago
Latest commit: 50193ef34edc12a54cc3de6865eb683d0f7ec244
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
Continuing to think about this, I'm not even sure if it makes sense to have separate crates at all
@ergoplatform/sigma-rust
will have nipopows
that depend on merkle-trees
so all the merkle-tree
types are going to end up in & exported from @ergoplatform/sigma-rust
anyway
Thank you for such a detailed explanation!
1. Adding a dependency on another package in the monorepo at the rust level causes `wasm-bindgen` to regenerate all the bindings and types again for the package that it's used. For example, `nipopow` package depends on the `BlockId` type in the `blockchain` package, if we added the dependency at the rust/cargo level on the `blockchain` crate it would add/duplicate all types in `blockchain` to the `nipopow` crate. Which makes sense in hindsight but defeats one of the main drivers of the monorepo to reduce bloat and causes the `nipopow` package to be polluted with `blockchain` types probably leading to more bloat than before
Does it happen only when you import BlockId
from blockchain
(wasm) crate? If it's a wasm-bindgen
, how about using BlockId
from the core ergo-chain-types
? I mean would serde serialization alone suffice for type to be used in wasm binding crate?
Would rust features offer any help? I mean more aggressively use them to "cut" crate into pieces and use only what's needed. We can introduce more features in the core crates if it'd help.
2. So I tried avoiding the dependency at the rust level and used imports from the npm packages in rust (content in this PR) which also has downsides: * Adds a lot of complexity * Requires duplicate declarations of rust WASM structs: https://github.com/ergoplatform/ergo-lib-wasm/compare/nipopow-crate?expand=1#diff-550c1a2647bb99e877fe8755550841820d5b176daf2841a66ef22174b9abf67fR1 * Adds overhead when a type in another monorepo crate needs to be used, we need to convert the `JsValue` to JSON then convert the JSON to the inner struct to avoid needing the WASM crate dependency/WASM struct: https://github.com/ergoplatform/ergo-lib-wasm/compare/nipopow-crate?expand=1#diff-e33ebcc48149b607b66c725649a9a5096a5d4aba19b50213e45da1778bc2936bR74 * The `Vec` methods are implemented at the wasm struct level so its unclear how we would work with vec of structs where the struct is from another crate - maybe ways to do this but it would be more hacks/complexity
This sound very complex.
So new thoughts on repo setup/package layout is: We only have separate packages when a package is self contained/doesn't depend on other crates in the monorepo. IMO we kind of need to have some kind of monolithic crate given the above points/current state of rust+wasm
So I'm thinking this for new npm package layout:
* `@ergoplatform/merkle-tree` (OK because self-contained) * `@ergoplatform/address` (Seems self-contained) * `@ergoplatform/sigma-rust` contains pretty much everything in `ergo-lib-wasm` now except for stuff that is self-contained and can be split out in the future.
I think there might be things we can do to improve DX with us using a monolithic package such as wrapping the generated wasm bindings in our own modules and re-exporting things so bundlers can use tree-shaking and we can export things logically (i.e
@ergoplatform/sigma-rust/wallet
) instead of everything from the package root - but would need experimentationCurrently no way to export rust code in modules using
wasm-bindgen
:* [Can't compile structs with the same name in different rust modules rustwasm/wasm-bindgen#2247](https://github.com/rustwasm/wasm-bindgen/issues/2247) * [How to have namespaces for Rust exported classes rustwasm/wasm-bindgen#2932](https://github.com/rustwasm/wasm-bindgen/issues/2932)
tl;dr it feels like monorepos containing inter-dependent packages with wasm-bindgen could be workable but it could add a lot of ongoing/immediate complexity to workaround wasm-bindgen and I'm not sure it is worth it
Any thoughts on all this @greenhat ?
I agree. Even if we end up shipping one big package there is still room for improving the binary size with wasm-opt
and no_std
.
- Does it happen only when you import
BlockId
fromblockchain
(wasm) crate? If it's awasm-bindgen
, how about usingBlockId
from the coreergo-chain-types
? I mean would serde serialization alone suffice for type to be used in wasm binding crate?
I think what you're suggesting is what I was doing here maybe? https://github.com/ergoplatform/ergo-lib-wasm/compare/nipopow-crate?expand=1#diff-e33ebcc48149b607b66c725649a9a5096a5d4aba19b50213e45da1778bc2936bR74 - if we're just going to use 1 crate this is no longer a issue
I agree. Even if we end up shipping one big package there is still room for improving the binary size with
wasm-opt
andno_std
.
Sounds good, one big package it is. Do you have a preference on what the npm package should be called? I.e @ergoplatform/sigma-rust
, @ergoplatform/ergo-lib-wasm
, etc?
- Does it happen only when you import
BlockId
fromblockchain
(wasm) crate? If it's awasm-bindgen
, how about usingBlockId
from the coreergo-chain-types
? I mean would serde serialization alone suffice for type to be used in wasm binding crate?I think what you're suggesting is what I was doing here maybe? https://github.com/ergoplatform/ergo-lib-wasm/compare/nipopow-crate?expand=1#diff-e33ebcc48149b607b66c725649a9a5096a5d4aba19b50213e45da1778bc2936bR74 - if we're just going to use 1 crate this is no longer a issue
Yes, but way simpler. For this case, I was thinking of using the BatchMerkleProof
from the core ergo-merkle-tree
crate. I naively thought that serde impl would suffice for a type to be used in a wasm crate without a wrapper type.
I agree. Even if we end up shipping one big package there is still room for improving the binary size with
wasm-opt
andno_std
.Sounds good, one big package it is. Do you have a preference on what the npm package should be called? I.e
@ergoplatform/sigma-rust
,@ergoplatform/ergo-lib-wasm
, etc?
Initially, the "umbrella" crate was called sigma-rust
since it was a sigmastate
"clone" but later, when more wallet-like and other features were added, I changed it to a broader ergo-lib
. I wonder if we should use @ergoplatform/ergo-lib-wasm
and change the major version. Would it be too much confusion for the developers?
I wonder if we should use
@ergoplatform/ergo-lib-wasm
and change the major version. Would it be too much confusion for the developers?
I think this sounds good
When we're ready here and publish the next major version we could use npm deprecate on the 2 current npm packages and direct devs to @ergoplatform/ergo-lib-wasm
- I'll add a "migrating from old npm packages/upgrading to 1.x.x" guide to the README at some stage before then
Great!
This was my initial approach to adding a
nipopow
andblockchain
package. Likely to be abandoned given I encountered a few things that made me reconsider the monorepo setup we discussed:wasm-bindgen
to regenerate all the bindings and types again for the package that it's used. For example,nipopow
package depends on theBlockId
type in theblockchain
package, if we added the dependency at the rust/cargo level on theblockchain
crate it would add/duplicate all types inblockchain
to thenipopow
crate. Which makes sense in hindsight but defeats one of the main drivers of the monorepo to reduce bloat and causes thenipopow
package to be polluted withblockchain
types probably leading to more bloat than beforeJsValue
to JSON then convert the JSON to the inner struct to avoid needing the WASM crate dependency/WASM struct: https://github.com/ergoplatform/ergo-lib-wasm/compare/nipopow-crate?expand=1#diff-e33ebcc48149b607b66c725649a9a5096a5d4aba19b50213e45da1778bc2936bR74Vec
methods are implemented at the wasm struct level so its unclear how we would work with vec of structs where the struct is from another crate - maybe ways to do this but it would be more hacks/complexitySo new thoughts on repo setup/package layout is: We only have separate packages when a package is self contained/doesn't depend on other crates in the monorepo. IMO we kind of need to have some kind of monolithic crate given the above points/current state of rust+wasm
So I'm thinking this for new npm package layout:
@ergoplatform/merkle-tree
(OK because self-contained)@ergoplatform/address
(Seems self-contained)@ergoplatform/sigma-rust
contains pretty much everything inergo-lib-wasm
now except for stuff that is self-contained and can be split out in the future.I think there might be things we can do to improve DX with us using a monolithic package such as wrapping the generated wasm bindings in our own modules and re-exporting things so bundlers can use tree-shaking and we can export things logically (i.e
@ergoplatform/sigma-rust/wallet
) instead of everything from the package root - but would need experimentationCurrently no way to export rust code in modules using
wasm-bindgen
:tl;dr it feels like monorepos containing inter-dependent packages with wasm-bindgen could be workable but it could add a lot of ongoing/immediate complexity to workaround wasm-bindgen and I'm not sure it is worth it
Any thoughts on all this @greenhat ?