stellar / rs-soroban-env

Rust environment for Soroban contracts.
Apache License 2.0
62 stars 43 forks source link

define rules and cleanup dependency tree to enable sustainable releases in the future #289

Closed MonsieurNicolas closed 1 year ago

MonsieurNicolas commented 2 years ago

To expedite development we've postponed looking at the dependency tree with "sustainability" in mind.

In particular we have not defined goals (potentially conflicting) for the different parts of the code.

Something like:

Those goals need to be reconciled with how the various components are organized, depend on each other (some dependency injection/inversion may be needed) and released.

MonsieurNicolas commented 2 years ago

Looking at a recent CVE in Go's bignum package, we may also want to ban certain APIs (that tend to create certain classes or security issues) for code that ends up at the core layer. For example, anything related to marshalling from/to strings as code in that space tends to do "way too much stuff" and it's probably not practical to code review all that code anyways. We'll need a linter of sorts to help ensure that those crates are "pure" in that respect.

leighmcculloch commented 2 years ago

Code that gets deployed as part of the SDK to support the "developer experience"

The test experience in the SDK is largely supported by the soroban-env-host crate, because the meaningful logic supporting testing is the Host, so while there is some functionality to support testing in the SDK, it is very minimal, so minimal I don't think it is worth the overhead of engineering a separation for.


The above mostly discusses the distinctions between the core layer and the SDK, however there are layers above the SDK that are much easier to separate from the core concerns. Most functionality in the SDK can be broken up into three parts:

Overall there seems to me to be an inherent tight coupling of the concerns in the SDK and the env crates, and significant coordination between the two, even though there exists solid interfaces between the two.

However, if we step away from the SDK, there are plenty of other projects that very easily distance from the guarantees of these components. For example, soroban-rpc, soroban-cli, dapp development, and all of the client libs are very far removed from the guarantees we need to provide in core or the SDK. It's much easier to iterate independently on those layers.

Here's a diagram illustrating existing dependencies and the types of concerns I see showing up in different areas:

soroban-whats-been-built-and-areas excalidraw

cc @graydon @tsachiherman @ire-and-curses

MonsieurNicolas commented 1 year ago

The test experience in the SDK is largely supported by the soroban-env-host crate, because the meaningful logic supporting testing is the Host, so while there is some functionality to support testing in the SDK, it is very minimal, so minimal I don't think it is worth the overhead of engineering a separation for.

currently yes (and I hope we can keep it that way), what I envision is that there should be things build on top: that's why I mentioned fuzzers, but I also imagine things like special hooks to facilitate verification.

In any case, what I think we need to do here is actually codify expectations for the different layers that @leighmcculloch identified so that we can properly trim & enforce the type of dependencies we allow in each crate (automation may follow, we first need to define the rules). I think this has some implications on code review & code pinning strategies and potentially on testing strategies too (core level changes are extremely heavy to implement and test and we don't want this outside of core).

leighmcculloch commented 1 year ago

trim & enforce the type of dependencies we allow in each crate

The real challenge with being selective will not be the direct dependencies we've picked, of which there are a few but reasonable number, but the deep tree of transitive dependencies.

For example, these are the direct dependencies we have today:

In total that's 40 unique dependencies.

Contrast that with 172 total dependencies in the rs-soroban-env repo.

We might be able to trim some of these. For example, the hex, num-traits, num-integer, num-derive, and itertools libraries could probably be replaced with a small amount of our own logic if necessary. Not confident at all on the remainder though.

graydon commented 1 year ago

That's an exaggeration:

IOW I would say that the host has 6 significant logical dependencies: wasmi, sha2+sha3, *-dalek, num-*, k256, rand_*.

graydon commented 1 year ago

Also dalek-curve25519 was actually only in there before for calibration -- because we used to define the costs of a signature verification with reference to the number of curve25519 operations it might do in the worst case -- and @jayz22 removed it when changing the calibration code. I actually ought to double check with @jayz22 on whether that's a conservative enough calibration (I'll file another bug) but in any case there should really only be at most one direct non-dev (non-calibration) dalek crate there.

graydon commented 1 year ago

(https://github.com/stellar/rs-soroban-env/pull/1031 trims deps a bit, https://github.com/stellar/rs-soroban-env/issues/1030 is about re-checking calibration)

graydon commented 1 year ago

Going back to the root of this bug though, I am not sure there's much actionable remaining here. We've done the following:

I don't know what else to do to the dependency tree to make env releases sustainable. The bug asks to "define rules" -- where? About what? If we have to take a new dep in the future surely we'll take it, but not without care and consideration and certainly not accidentally (any more than we carelessly add deps to core normally; we think through whether we can avoid it and only add them when they're necessary and obviously better than writing it ourselves would be). Should I just write that general guideline down somewhere?

graydon commented 1 year ago

Closing, I think this is as done as it's going to get.