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.

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

Adding deliverables document for MoveVM Substrate Pallet part 1 #974

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

MeerKatDev
Copy link
Contributor

@MeerKatDev MeerKatDev commented Aug 10, 2023

Milestone Delivery Checklist

  • The milestone-delivery-template.md has been copied and updated.
  • The invoice form 📝 has been filled out for this milestone.
  • This pull request is being made by the same account as the accepted application.
  • I have disclosed any and all sources of reused code in the submitted repositories and have done my due diligence to meet its license requirements.
  • In case of acceptance, the payment will be transferred to the BTC/ETH/fiat account provided in the application.
  • The delivery is according to the Guidelines for Milestone Deliverables.

Link to the application pull request: w3f/Grants-Program#1769

@Whisker17
Copy link
Contributor

Hey @MeerKatDev, good to see your contribution! I've finished my external evaluation here #975.

As you can see from my evaluation, in general, I will accept your milestone delivery, but still have some small questions:

  1. You set many unit tests that were ignored, can I understand that since milestone 1 was an assessment and inquiry in nature, it was not implemented?
  2. There doesn't seem to be any valid information on doc-tests either, will it also be implemented in m2?

Could you please clarify my questions? Appreciate it!

@MeerKatDev
Copy link
Contributor Author

Hey @MeerKatDev, good to see your contribution! I've finished my external evaluation here #975.

As you can see from my evaluation, in general, I will accept your milestone delivery, but still have some small questions:

Hi @Whisker17 , thanks for the rapid response, appreciate it a lot!

  1. You set many unit tests that were ignored, can I understand that since milestone 1 was an assessment and inquiry in nature, it was not implemented?

We have set up many integration tests (under the tests subdirectory), which fail now as they are only stubs.
We have explained a bit in PR #45 - all RPCs, extrinsics and other functions are stubs in the Milestone 1, because the MoveVM is not ported, and it is not working inside the pallet. That makes us unable to perform actual integration tests.

However, to show how it will look in the future, we added some test stubs and the Substrate runtime mock, which is running correctly and configuring testing runtime. We added unit tests where we could perform them without executing the Substrate runtime (or its mock). We will add more integration tests and unit tests in the next milestones.

  1. There doesn't seem to be any valid information on doc-tests either, will it also be implemented in m2?

Yes, we mentioned doctests in the Testing Guide, which covers our approach to the testing part in general (not only in Milestone 1 but for the whole project).

As mentioned in our grant proposal for Milestone 1, we would implement stubs for the same set of APIs that Pontem did, and afterwards, we would add or remove new calls (RPC, extrinsics, etc.). That is why we found it less valuable for the project to provide extensive doctests during the planning and prototyping phase, since much can change during Milestone 2. Thus, as stated in the Testing Guide, we consider doctests as part of the project, and we will provide them when we are sure that the functions will not be removed and will be exposed to the future user.

@Whisker17
Copy link
Contributor

Yeah, good to hear that, so this m1 delivery is good to me, looking forward to your m2!

@semuelle semuelle self-assigned this Aug 11, 2023
Copy link
Member

@semuelle semuelle left a comment

Choose a reason for hiding this comment

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

Hi @MeerKatDev. I am trying to verify the evaluation and struggle to get the move-cli compiled. docker build -t move/cli -f docker/move-cli/Dockerfile . fails with the following errors. Any idea what's missing?

156.3 error[E0277]: the trait bound `U256FromStrError: StdError` is not satisfied
156.3    --> language/move-command-line-common/src/parser.rs:197:50
156.3     |
156.3 197 |                 let (u, _) = parse_u256(contents)?;
156.3     |                                                  ^ the trait `StdError` is not implemented for `U256FromStrError`
156.3     |
156.3     = help: the following other types implement trait `FromResidual<R>`:
156.3               <Result<T, F> as FromResidual<Result<Infallible, E>>>
156.3               <Result<T, F> as FromResidual<Yeet<E>>>
156.3     = note: required for `anyhow::Error` to implement `From<U256FromStrError>`
156.3     = note: required for `Result<ParsedValue<Extra>, anyhow::Error>` to implement `FromResidual<Result<Infallible, U256FromStrError>>`
156.3 
156.3 error[E0277]: the trait bound `U256FromStrError: StdError` is not satisfied
156.3    --> language/move-command-line-common/src/parser.rs:217:84
156.3     |
156.3 217 |                     let (u, _) = parse_u256(contents.strip_suffix("u256").unwrap())?;
156.3     |                                                                                    ^ the trait `StdError` is not implemented for `U256FromStrError`
156.3     |
156.3     = help: the following other types implement trait `FromResidual<R>`:
156.3               <Result<T, F> as FromResidual<Result<Infallible, E>>>
156.3               <Result<T, F> as FromResidual<Yeet<E>>>
156.3     = note: required for `anyhow::Error` to implement `From<U256FromStrError>`
156.3     = note: required for `Result<ParsedValue<Extra>, anyhow::Error>` to implement `FromResidual<Result<Infallible, U256FromStrError>>`
156.3 
156.4 error[E0277]: the trait bound `U256CastError: StdError` is not satisfied
156.4    --> language/move-command-line-common/src/values.rs:315:76
156.4     |
156.4 315 |                 Extra::move_value_into_concrete(MoveValue::U64(u.try_into()?))
156.4     |                                                                            ^ the trait `StdError` is not implemented for `U256CastError`
156.4     |
156.4     = help: the following other types implement trait `FromResidual<R>`:
156.4               <Result<T, F> as FromResidual<Result<Infallible, E>>>
156.4               <Result<T, F> as FromResidual<Yeet<E>>>
156.4     = note: required for `anyhow::Error` to implement `From<U256CastError>`
156.4     = note: required for `Result<<Extra as ParsableValue>::ConcreteValue, anyhow::Error>` to implement `FromResidual<Result<Infallible, U256CastError>>`
156.4 
156.4 For more information about this error, try `rustc --explain E0277`.
156.4 error: could not compile `move-command-line-common` due to 3 previous errors
156.4 warning: build failed, waiting for other jobs to finish...
------
Dockerfile:39
--------------------
  37 |         && [ -x "$(set -x; command -v npm)" ]
  38 |     
  39 | >>> RUN cd /move; cargo build -p move-cli
  40 |     
  41 |     ENTRYPOINT ["/move/target/debug/move"]
--------------------
ERROR: failed to solve: process "/bin/sh -c cd /move; cargo build -p move-cli" did not complete successfully: exit code: 101

@Whisker17
Copy link
Contributor

I am trying to verify the evaluation and struggle to get the move-cli compiled.

Hey @semuelle, have you ever tried to build from the source? I built move-cli by following this tutorial, and it worked.

@Rqnsom
Copy link

Rqnsom commented Aug 17, 2023

This is an expected failure - we modified only the move-core-types crate (and the bcs crate) in the move language fork repo - to see how much work it would take to make it compatible with Substrate.

It was more a proof of concept (PoC) thing we did to have better time estimates for the second Milestone. The move compiler depends on move-core-types, and without our PoC change - it would build successfully.
Making all necessary Move crates no-std compatible while keeping the whole Move ecosystem fully functional is something we plan to do in the next Milestone.

@semuelle
Copy link
Member

Sorry for the late reply, @MeerKatDev. I have reviewed and confirmed the external evaluation. I very much appreciate the thorough design documentation. Your milestone is hereby accepted.

I have forwarded your invoice for processing.

@semuelle semuelle merged commit 7932b07 into w3f:master Aug 23, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants