TritonVM / tasm-lib

A collection of functions written in Triton VM assembly (tasm)
Apache License 2.0
11 stars 2 forks source link

Triton and twenty first re exports pr #68

Closed dan-da closed 7 months ago

dan-da commented 8 months ago

For your consideration.

As I understand it, the dependency tree for triton/neptune crates logically looks like this:

neptune-core
 |- tasm-lib
      |- triton-vm
           |- twenty-first
               |- bfieldcodec_derive

and maybe some more, but those are the one's I'm most interested in right now. But instead here is the actual output of cargo tree, with version info stripped:

$ cargo tree --no-dedupe -f "{lib}" | grep "neptune\|twenty\|triton\|tasm\|bfield"
neptune_core
├── tasm_lib
│   ├── derive_tasm_object
│   ├── triton_vm
│   │   ├── twenty_first
│   │   │   ├── bfieldcodec_derive
│   └── twenty_first
│       ├── bfieldcodec_derive
├── triton_vm
│   ├── twenty_first
│   │   ├── bfieldcodec_derive
├── twenty_first
│   ├── bfieldcodec_derive

This PR is aimed at moving us towards the first dep tree.

This PR does two things, in two separate commits:

  1. re-exports triton_vm and triton_vm::twenty_first in lib.rs like so:
    pub use triton_vm;
    pub use triton_vm::twenty_first; // less verbosity
  2. changes all use twenty_first:: statements to use crate::twenty_first.

The purpose of [1] is to help out crates that depend on us (eg neptune-core), so they don't have to specify a version of triton_vm or twenty_first, but automatically get our version.

The purpose of [2] is so that we are using the twenty_first from triton_vm and don't need to have a dep of our own.

Unfortunately [2] is complicated by a strange situation with bfieldcodec_derive::BFieldCodec, which hard-codes string references to ::twenty_first in its derive macro, forcing crates that use it to have a dep on twenty-first. So until that is resolved tasm-lib won't compile without a twenty-first dep, and thus I left it in.

As for resolving it, the simplest thing, to my mind, is to move bfieldcodec_derive::BFieldCodec into twenty-first. But then I do not know any history of why it is a separate crate, or why I can't seem to find the repo for it.

[2] is a much bigger change, as it touches a lot of source files. I made this separate commits, so that if [2] is not accepted, hopefully we can still get [1] merged.

my motivation:

Mainly just to cleanup deps, and minimize dependency-hell in accordance with rust best practices.

But also, I have in the past encountered conflicts in neptune-core where this would have helped.


Final thought: this PR is just for triton_vm and twenty_first that I know are used in the public API. There may well be types from other crates exposed in the API. So best practice would be to review the public API for such cases.

Sword-Smith commented 8 months ago

bfieldcodec is located here: https://github.com/Neptune-Crypto/twenty-first/tree/master/bfieldcodec_derive

Maybe this line and its explanation provide some relevant info about the twenty_first hard-coded crate name. https://github.com/Neptune-Crypto/twenty-first/blob/10afd069f0de8de0d693bca291fb0f06b4961a16/twenty-first/src/lib.rs#L19

dan-da commented 8 months ago

bfieldcodec is located here: https://github.com/Neptune-Crypto/twenty-first/tree/master/bfieldcodec_derive

thx!

Maybe this line and its explanation provide some relevant info about the twenty_first hard-coded crate name. https://github.com/Neptune-Crypto/twenty-first/blob/10afd069f0de8de0d693bca291fb0f06b4961a16/twenty-first/src/lib.rs#L19

yes I read that comment before. It speaks to intent, but not really why a derive macro needs to be hard-coding a crate name. I've never written a derive macro, so I can't speak to what is or isn't possible. It's just that I don't believe I've ever seen behavior like that before in any macro from a public crate, so I feel like there must be another way to do it.

That said, I have an idea for a work-around to achieve my goal without altering the macro much. Basically, I just replaced all ::twenty-first in the macro with crate::twenty-first. Then, so long as the depending crate places a use twenty_first in lib.rs, it should work (I hope). And that could actually be a use triton_vm::twenty_first, for example. So no Cargo.toml dep required. So far the bfieldcodec_derive crate compiles fine with that change, but I haven't tried to build any dependent crate yet. getting late here, will try in the am.

dan-da commented 8 months ago

yeah, so as commented in https://github.com/Neptune-Crypto/twenty-first/pull/180, this approach is working, so I think we should go ahead and merge both PRs.

$ cargo tree --no-dedupe -f "{lib}" | grep "triton\|twenty\|neptune\|tasm_lib\|bfield"
tasm_lib
└── triton_vm
    ├── twenty_first
    │   ├── bfieldcodec_derive

But let me know if there are concerns. I will wait until Monday if I don't hear anything before.