Thanks for the delivery. We will look into it as soon as possible.

It's bigest harvest

I haven't been able to build the Resource Viewer with the json-schema feature:

error[E0432]: unresolved import `crate::language_storage::CORE_CODE_ADDRESS`
   --> /Users/marcin/.cargo/git/checkouts/libra-d0537194d08a494c/ca5f70d/language/move-core/types/src/vm_status.rs:316:38
    |
316 |         language_storage::{ModuleId, CORE_CODE_ADDRESS},
    |                                      ^^^^^^^^^^^^^^^^^ no `CORE_CODE_ADDRESS` in `language_storage`

error[E0425]: cannot find value `CORE_CODE_ADDRESS` in module `crate::language_storage`
  --> /Users/marcin/.cargo/git/checkouts/libra-d0537194d08a494c/ca5f70d/language/move-core/types/src/move_resource.rs:31:47
   |
31 |             address: crate::language_storage::CORE_CODE_ADDRESS,
   |                                               ^^^^^^^^^^^^^^^^^ not found in `crate::language_storage`

I'm using rustc 1.53.0-nightly (5d04957a4 2021-03-22)

Any ideas? It works without the json-schema feature, using cargo install --git https://github.com/pontem-network/move-tools.git move-resource-viewer --no-default-features --features="ps_address"

UPDATE:
Building worked from inside the resource-viewer directory by executing: cargo install --path . --no-default-features --features="ps_address, json-schema"

Another issue: I haven't been able to publish the standard modules by following the steps here. I'm getting the following error:

Mar 23 11:06:36.012 DEBUG apply_extrinsic:publish_std: publish result: VmResult { status_code: MALFORMED_CONSTANT_DATA, gas_used: 2 }    {}

Let me know if you can help!

@mmagician thank you for your report, we are looking into issues right now.

@mmagician We just checked everything and did several fixes, mostly in documentation.

1. Fixed move-resource-viewer installation command.
2. Please, be sure to use the latest dove, we recommend to reinstall it from move-tools repository by building from source code.
3. Rebuild all modules/scripts using latest dove.
4. When you deploy the standard library, please, follow ordering, like in the documentation.

If you have any future questions please feel free to ask me.

That worked, thanks. I'll continue with testing

@borispovod Below you'll find further feedback:

Test coverage - no access to https://github.com/fzzr-/sp-move/, so I can't really know what was tested. Could you please make this more transparent?

Could you allow specifying output filename for dove ct, for creating multiple Txs each with a different input. The workaround is to create multiple scripts, changing method names, but that's not very elegant. Just a suggestion :)

I was initially a little bit confused by the following experiment:

• Create a module in Bob's account to Store a u64 value (example from your README) & deploy it
• Create a script that stores something
• Call the script from Alice's account
• Result: the value is stored under ::Store::u64, whereas I was expecting it to be under

After reading through the your docs, this made sense, it was simply counter-intuitive the first time.
Nevertheless, one question remains: does that mean that once a module is deployed to B's account, anyone can always call its public methods?
Is there a way to only allow calls to the module by the owner? I'm thinking of something like module access control (e.g. private module when declaring it). I haven't found such references in the official diem docs.
I imagine that the way around it is inserting logic into each method that, when desired, checks whether the caller is the owner and otherwise aborts execution, sort of like the Ownable contract. Anyway, let me know your thoughts on this.

Some stdlib modules are largely copied from the diem stdlib repo. Since it's not a direct fork, please make sure to include an attribution like you have for the VM.

Move resource viewer: nice, as promised in the last milestone. I wonder if, instead of always writing to a file, one could get the output in stdout? That would help for debugging as then you can easily parse it with jq without creating new files. Again, just a suggestion.

The deliverable 0: Library that converts Move WriteSet in compatible format for Polkadot storage (LCS → Polkadot codec). has not been met, and at this point I wouldn't push for implementing it if indeed it's too computationally expensive. Since this requirement has changed, I would kindly ask you to submit a PR to the original contract to remove that deliverable & reduce the price accordingly (since there were initially 6 core deliverables 0-5 inclusive, I'd say 1/6th of the price - I'm excluding the resource viewer from the count, as it was originally part of M3. I hope this sounds fair?)

The deliverable 1: WriteSets processing: Process WriteSets from MoveVM to Polka storage. Read/Write operations, Del operations. I'm not entirely sure which part of the commit is doing that?

Furthermore, your delivery report states: Move Pallet saving result of transaction execution in pallet storage - which again I haven't been able to find. I only see that you're returning the results, but not saving it to pallet storage.

@mmagician

Test coverage - no access to https://github.com/fzzr-/sp-move/, so I can't really know what was tested. Could you please make this more transparent?

@fzzr- fork was removed, because not needed anymore (he just tested coverage in his own fork and i used his link to show coverage), than we moved coverage pipeline to our Github, and run it there, you can see how it was, but again removed it because of limitations of Github CI worker (currently our devops up our own CI worker on our servers). We just tried now to make instruction for you to run it locally, but unfortunately we are getting error during building pallet for grcov:

Compiling sp-api v2.0.1
error[E0432]: unresolved import `sp_core::to_substrate_wasm_fn_return_value`
  -->/.cargo/registry/src/github.com-1ecc6299db9ec823/sp-api-2.0.1/src/lib.rs:54:9
   |
54 | pub use sp_core::to_substrate_wasm_fn_return_value;
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `to_substrate_wasm_fn_return_value` in the root

Command to reproduce:

cargo install grcov
SKIP_WASM_BUILD=1 RUSTFLAGS="-Zprofile -Ccodegen-units=1 -Cinline-threshold=0 -Clink-dead-code -Coverflow-checks=off -Cpanic=abort -Zpanic_abort_tests" cargo test -p=sp-mvm --all-features --no-fail-fast
grcov ./target/debug --llvm --branch --output-path ./target/cov --output-type=html

Seems the issue fixed in Substrate 3.0.0. Currently we are going to update our pallet to substrate 3.0.0 and after it we can show coverage again.

Could you allow specifying output filename for dove ct, for creating multiple Txs each with a different input. The workaround is to create multiple scripts, changing method names, but that's not very elegant. Just a suggestion :)

Sure. I just added it to our Jira and we will implement it in the near time. If you want I can update the next milestone with it if needed.

I was initially a little bit confused by the following experiment:
Create a module in Bob's account to Store a u64 value (example from your README) & deploy it
Create a script that stores something
Call the script from Alice's account
Result: the value is stored under ::Store::u64, whereas I was expecting it to be under
After reading through the your docs, this made sense, it was simply counter-intuitive the first time.
Nevertheless, one question remains: does that mean that once a module is deployed to B's account, anyone can always call its public methods?
Is there a way to only allow calls to the module by the owner? I'm thinking of something like module access control (e.g. private module when declaring it). I haven't found such references in the official diem docs.
I imagine that the way around it is inserting logic into each method that, when desired, checks whether the caller is the owner and otherwise aborts execution, sort of like the Ownable contract. Anyway, let me know your thoughts on this.

Well, anyone can call a public method of any module. You can add something like owner to the module, by constant or by store owner address into a specific resource and check if the passed signer address matches the owner. For example look at this module i wrote for you:

address 5FHneW46xGXgs5mUiveU4sbTyGBzmstUspZC92UhjJM694ty {
module OnlyOwnerStore {
    use 0x01::Signer;
    resource struct U64 {
        val: u64
    }
    const OWNER : address = 5FHneW46xGXgs5mUiveU4sbTyGBzmstUspZC92UhjJM694ty;
    public fun store_u64(account: &signer, val: u64) {
        assert(Signer::address_of(account) == OWNER, 101); // Throw error if addresses don't match.
        let foo = U64 { val: val };
        move_to<U64>(account, foo);
    }
}
}

It verifies if the signer is the owner of the module and allows the U64 number to be stored. Also, read about signers in Diem Move documentation.

Some stdlib modules are largely copied from the diem stdlib repo. Since it's not a direct fork, please make sure to include an attribution like you have for the VM.

Done. See commit.

Move resource viewer: nice, as promised in the last milestone. I wonder if, instead of always writing to a file, one could get the output in stdout? That would help for debugging as then you can easily parse it with jq without creating new files. Again, just a suggestion.

Already in progress, will be ready in the near time.

The deliverable 0: Library that converts Move WriteSet in compatible format for Polkadot storage (LCS → Polkadot codec). has not been met, and at this point I wouldn't push for implementing it if indeed it's too computationally expensive. Since this requirement has changed, I would kindly ask you to submit a PR to the original contract to remove that deliverable & reduce the price accordingly (since there were initially 6 core deliverables 0-5 inclusive, I'd say 1/6th of the price - I'm excluding the resource viewer from the count, as it was originally part of M3. I hope this sounds fair?)

We are not implementing it, because:

• We can't extract profit from it, as the default Polka storage viewer wouldn't work anyway, same with other related functionalities, because Move types are not fully compatible with Polkadot.
• It will increase storage and gas, as resources in Move could be very complex, and for each generic/field of resource we will need to add additional byte (to store tag type).
• At the same time it's complex and a lot of work.

I'm going to update PR according to your recommendations, it's fine and fair :)

The deliverable 1: WriteSets processing: Process WriteSets from MoveVM to Polka storage. Read/Write operations, Del operations. I'm not entirely sure which part of the commit is doing that?
Furthermore, your delivery report states: Move Pallet saving result of transaction execution in pallet storage - which again I haven't been able to find. I only see that you're returning the results, but not saving it to pallet storage.

When I put it into the 2nd milestone I expected we are going to implement only dry-run (means without storage and processing WriteSets, and result of VM execution) mode in 1st milestone, but we've done it already in 1st milestone.
For processing WriteSets we've done storage adapter for VM. You can see how it works inside VM. It's done for WriteSets processing.

About results, we are return result from VM execution, it's true, but before we convert VM results using from_vm_results to Polkadot ones.

@borispovod Thanks for a comprehensive explanation.

First, regarding coverage: it's fine by me that we proceed with M2 before you migrate to substate version 3.0. Your test folder contains a fairly good set of cases, although I've created a PR to remove the binary targets & instead call the build_assets.sh script, which I believe is a cleaner practice and allows for better transparency.

Sounds great that you're already working on the improvements to tooling, it's appreciated. The suggestion regarding dove ct isn't mandatory, but sure, if you want to implement it in the next milestone - bonus "reputation" points for you :) (currently only tracked internally, but who knows, maybe they'll be on-chain one day!)

I'm going to update PR according to your recommendations, it's fine and fair :)

I'll await the PR to amend the scope & price of the original contract, then.

but we've done it already in 1st milestone.

Yes, I thought that was the case, but just wanted to make sure we're on the same page

About results, we are return result from VM execution, it's true, but before we convert VM results using from_vm_results to Polkadot ones.

That's fine, but it still doesn't mean you're saving the results in pallet storage, right? Could you update the relevant deliverable: Move Pallet saving result of transaction execution in pallet storage if indeed that's the case?

@mmagician ,

We've merged your PR, really appreciate your help and feedback.

Move Pallet saving result of transaction execution in pallet storage

I updated the current delivery: there are links to code containing VM results handling, WriteSets processing and Storage adapter, and changed description, so it should be much clearer than the previous version.

I see my PR to Open Grants Program already merged.
Also, i sent updated invoice, so your accountant team should receive it shortly.

Thank you!

@borispovod Congratulations on completing the second milestone! You'll find my evaluation notes here.

hi @borispovod we transferred the payment.

@RouvenP we got transfer, thank you!