[RFC] Add Starcoin-framework as a dependency

[666lcz]

Per this suggestion, we will add Starcoin-framework as a dependency for the u256 support.

TODO(once we solve the compiler error)

• also add the dependency to sui_programmability/framework/ where we will define an ERC721 module
• Add an example module that uses u256

[666lcz]

@sblackshear @awelc @lxfind , I am getting the following compiler error:

error[E02001]: duplicate declaration, item, or annotation
  ┌─ /var/folders/g4/ycgy0bpj5qz3_rzk__h2l5g40000gn/T/.tmpGYS4YR/mv_interfaces/06159891049857472948/0000000000000000000000000000000000000001/Errors.move:1:13
  │
1 │ module Std::Errors {
  │             ^^^^^^
  │             │
  │             Duplicate definition for module 'Std::Errors'
  │             Module previously defined here

error[E02001]: duplicate declaration, item, or annotation
  ┌─ /var/folders/g4/ycgy0bpj5qz3_rzk__h2l5g40000gn/T/.tmpGYS4YR/mv_interfaces/06159891049857472948/0000000000000000000000000000000000000001/FixedPoint32.move:1:13
  │
1 │ module Std::FixedPoint32 {
  │             ^^^^^^^^^^^^
  │             │
  │             Duplicate definition for module 'Std::FixedPoint32'
  │             Module previously defined here

error[E02001]: duplicate declaration, item, or annotation
  ┌─ /var/folders/g4/ycgy0bpj5qz3_rzk__h2l5g40000gn/T/.tmpGYS4YR/mv_interfaces/06159891049857472948/0000000000000000000000000000000000000001/Hash.move:1:13
  │
1 │ module Std::Hash {
  │             ^^^^
  │             │
  │             Duplicate definition for module 'Std::Hash'
  │             Module previously defined here

error[E02001]: duplicate declaration, item, or annotation
  ┌─ /var/folders/g4/ycgy0bpj5qz3_rzk__h2l5g40000gn/T/.tmpGYS4YR/mv_interfaces/06159891049857472948/0000000000000000000000000000000000000001/Vector.move:1:13
  │
1 │ module Std::Vector {
  │             ^^^^^^
  │             │
  │             Duplicate definition for module 'Std::Vector'
  │             Module previously defined here

error[E02001]: duplicate declaration, item, or annotation
  ┌─ /var/folders/g4/ycgy0bpj5qz3_rzk__h2l5g40000gn/T/.tmpGYS4YR/mv_interfaces/06159891049857472948/0000000000000000000000000000000000000001/Option.move:1:13
  │
1 │ module Std::Option {
  │             ^^^^^^
  │             │
  │             Duplicate definition for module 'Std::Option'
  │             Module previously defined here

error[E03003]: unbound module member
    ┌─ /Users/cl/ml/fastnft/sui_programmability/framework/../examples/sources/Hero.move:187:9
    │
187 │         Option::swap_or_fill(&mut hero.sword, new_sword)
    │         ^^^^^^^^^^^^^^^^^^^^ Invalid module access. Unbound function 'swap_or_fill' in module 'Std::Option'

error[E02001]: duplicate declaration, item, or annotation
  ┌─ /var/folders/g4/ycgy0bpj5qz3_rzk__h2l5g40000gn/T/.tmpGYS4YR/mv_interfaces/06159891049857472948/0000000000000000000000000000000000000001/Signer.move:1:13
  │
1 │ module Std::Signer {
  │             ^^^^^^
  │             │
  │             Duplicate definition for module 'Std::Signer'
  │             Module previously defined here

error[E02001]: duplicate declaration, item, or annotation
  ┌─ /var/folders/g4/ycgy0bpj5qz3_rzk__h2l5g40000gn/T/.tmpGYS4YR/mv_interfaces/06159891049857472948/0000000000000000000000000000000000000001/BCS.move:1:13
  │
1 │ module Std::BCS {
  │             ^^^
  │             │
  │             Duplicate definition for module 'Std::BCS'
  │             Module previously defined here

Let me know if you have any insights on resolving this

[awelc]

I suspect that you are seeing the conflict because we import Move's standard lib and so does Starcoin (and it looks like you pulling all their framework code).

I am not sure what's possible in Move.toml files, but in Cargo.toml (e.g. here) you can pull more fine-grain dependencies separately.

[lxfind]

I think we may have to copy the u256.move in for now.
We cannot directly depend on their package because they put everything (including the original move stdlib) in the same address (same as our std). So there will be collisions.
I wonder if we could encourage the starcoin team to add u256 to move stdlib proper.

[oxade]

@666lcz create an issue for tracking U256 support because we'll also have to propagate this to dependent services, which are currently capped at U64 now.

[sblackshear]

I think we may have to copy the u256.move in for now. We cannot directly depend on their package because they put everything (including the original move stdlib) in the same address (same as our std). So there will be collisions. I wonder if we could encourage the starcoin team to add u256 to move stdlib proper.

Agreed with copying this in for now + encouraging the Move team to include this in the stdlib (assuming they don't go even further and add it to the language itself...). We actually do this with the Move stdlib as well (instead of depending on it in GH) to improve CI performance.

[666lcz]

After copying the files, I encountered the following error when running cargo test:

Failures in 0x0::Arithmetic:

┌── test_add ──────
│ ITE: An unknown error was reported. Location:
│ VMError (if there is one): VMError {
│     major_status: UNEXPECTED_VERIFIER_ERROR,
│     sub_status: None,
│     message: Some(
│         "Unexpected verifier/deserialization error! This likely means there is code stored on chain that is unverifiable!\nError: VMError { major_status: MISSING_DEPENDENCY, sub_status: None, message: None, location: Module(ModuleId { address: 0000000000000000000000000000000000000003, name: Identifier(\"U256\") }), indices: [(FunctionHandle, 0)], offsets: [] }",
│     ),
│     location: Module(
│         ModuleId {
│             address: 0000000000000000000000000000000000000003,
│             name: Identifier(
│                 "U256",
│             ),
│         },
│     ),
│     indices: [
│         (
│             FunctionHandle,
│             0,
│         ),
│     ],
│     offsets: [],
│ }
└──────────────────

Test result: FAILED. Total tests: 3; passed: 2; failed: 1
test run_examples_move_unit_tests ... FAILED

failures:

---- run_examples_move_unit_tests stdout ----
thread 'run_examples_move_unit_tests' panicked at 'called `Result::unwrap()` on an `Err` value: MoveUnitTestFailure { error: "Test failed" }', sui_programmability/framework/src/lib.rs:209:32
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    run_examples_move_unit_tests

test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.86s

I suspect this error is because I did not copy the native function definition from Starcoin.

I have the following questions:

1. Note that currently I am copying the move files into https://github.com/MystenLabs/fastnft/tree/90634c7f2b6fc4111b5ff2720613cc22a95f1330/sui_programmability/framework/deps/starcoin-framework, is this the right approach?
2. should I also copy these two files(f1, f2)?
3. Where should I put the native function definitions? Should I put them inside https://github.com/MystenLabs/fastnft/tree/main/sui_programmability/framework/src/natives ?

cc @sblackshear @awelc @lxfind

[lxfind]

In order to make a new package accessible on-chain without publishing it, the package must be included in the genesis.
I don't have a strong opinion between adding this as a genesis package, or simply put into sui framework.

Looks like we do need the natives and yet that's the correct path.

[666lcz]

It turns out it's tricky to add/copy the native part: https://github.com/starcoinorg/starcoin/tree/13d2c9ee658bebf9d7297ad498c7efc161aa0a17/vm/natives. I tried two approaches:

Approach 1: add https://github.com/starcoinorg/starcoin/tree/13d2c9ee658bebf9d7297ad498c7efc161aa0a17/vm/natives as a dependency in cargo.toml. I encounter many errors like the following

➜ cargo build
    Blocking waiting for file lock on package cache
    Updating git repository `https://github.com/starcoinorg/starcoin`
    Updating git repository `https://github.com/starcoinorg/diem`
    Updating crates.io index
error: failed to select a version for `futures-util`.
    ... required by package `diem-workspace-hack v0.1.0 (https://github.com/starcoinorg/diem?rev=6ff224f42a408f584acbcd37eebac14de619b16f#6ff224f4)`
    ... which satisfies git dependency `diem-workspace-hack` of package `bytecode v0.1.0 (https://github.com/starcoinorg/diem?rev=6ff224f42a408f584acbcd37eebac14de619b16f#6ff224f4)`
    ... which satisfies git dependency `bytecode` of package `docgen v0.1.0 (https://github.com/starcoinorg/diem?rev=6ff224f42a408f584acbcd37eebac14de619b16f#6ff224f4)`
    ... which satisfies git dependency `docgen` of package `move-prover v0.1.0 (https://github.com/starcoinorg/diem?rev=6ff224f42a408f584acbcd37eebac14de619b16f#6ff224f4)`
    ... which satisfies git dependency `move-prover` of package `starcoin-natives v1.8.0-alpha (https://github.com/starcoinorg/starcoin?rev=094a71324f327d74546a609347f8b31374c627c9#094a7132)`
    ... which satisfies git dependency `starcoin-natives` of package `sui-framework v0.1.0 (/Users/cl/ml/fastnft/sui_programmability/framework)`
    ... which satisfies path dependency `sui-framework` (locked to 0.1.0) of package `sui v0.1.0 (/Users/cl/ml/fastnft/sui)`
versions that meet the requirements `^0.3.16` are: 0.3.21

the package `diem-workspace-hack` depends on `futures-util`, with features: `proc-macro-hack` but `futures-util` does not have these features.

I tried with earlier revisions(from five days ago to 6 months ago), but they all have dependency version conflicts.

Note that most of the conflicts are due to this diem-workspace-hack, which is a crate in https://github.com/diem/diem/blob/fb173cd45c866b045c77866f6ad86b19744df548/crates/diem-workspace-hack/README.md#L4. If we can find a way to bypass this, this approach should work.

Approach 2: manually copy relevant packages to local repo (and hopefully manually correct the version)

The problem is that each package in the repo is connected to many others: e.g., https://github.com/starcoinorg/starcoin/blob/13d2c9ee658bebf9d7297ad498c7efc161aa0a17/vm/natives/Cargo.toml#L19-L29 . We need to copy a lot of packages if we take this approach.

Next steps

We actually do not need all these u256 functionalities(add, multiply) for the ERC721 implementation. We only need a way to store the bytes and compare the bytes. Moreover, most, if not all, popular NFT have a small token id (e.g., BAYC 1-10000, Azuki 1-10000). Therefore, to unblock the development, I am proposing

1. Pause the development of this PR for now(feel free to pick this up if you are interested)
2. In the first draft of the ERC721 module, use u128 to represent tokenID for simplicity
3. After we complete the MVP, build a proper implementation of U256(either by building a barebone version that does not support arithmetic operations, or porting/replicating starcoin's implementation)

[sblackshear]

Your proposal makes perfect sense to me. Thanks for bearing with the difficulty of importing this!

[lxfind]

We should bring this up in this week's Move meeting with the Diem team. It seems to me the amount of work is similar to just adding U256 to upstream move stdlib.
Does the Diem team need Starcoin's permission to get this into their stdlib? Or should this be initiated by the starcoin team?

[huitseeker]

Here's the best documentation of what the diem-worspace-hack crate is for, and why it's firing off so many dependency errors (by the person who made it happen):
https://docs.rs/cargo-hakari/latest/cargo_hakari/about/index.html

As a consequence, I think it will not be possible to align the versions in the starcoin and move dependencies, and that we should give up on importing this via a regular cargo dependency.

The only solutions I can see are an upstream contribution (add what we need to Move) or a fork.