External evaluation of openPayroll milestone 1

[dastansam]

Faced some difficulties with running tests

Application: https://github.com/w3f/Grants-Program/blob/master/applications/openPayroll.md
Milestone delivery PR: #939

[Whisker17]

Hey @dastansam , nice work!

I think for the web test, you need to use a talisman or subwallet extension, and then you can pass the tests. See here:

And I also met the contract build error, and I think the problem should be there is no v0.30.1 release for this library.

[0xLucca]

Hey @dastansam!

Thank you for providing feedback on the project. We have investigated the issues raised and have some explanations and solutions to address them.

Regarding the problem with building and testing the smart contract using Docker, it appears that the issue was caused by a recent release of cargo-contract, the tool used for compiling smart contracts. Specifically, a new version of cargo-contract was released two days ago, which might have introduced some incompatibilities or bugs. To resolve this, we have taken the approach of using fixed versions for Rust and cargo-contract instead of relying on the latest versions. This should ensure a stable and consistent build environment for the smart contract using the Docker image.

Regarding the usage of unsafe methods in the smart contract, we acknowledge the concern. However, we want to clarify that in all the places where we are using unwrap(), we have carefully validated the conditions before calling these methods. This means that we are confident that they will never panic.

If you have any further concerns or suggestions, please don't hesitate to let us know.

[dastansam]

hey @0xLucca,

thank you for the swift response. I agree that unwrap issue is not critical and can confirm that contract compilation error with Docker is fixed now. There is another issue with running unit tests, though. Am I missing some step before running tests?

However, we want to clarify that in all the places where we are using unwrap(), we have carefully validated the conditions before calling these methods.

yes, I see that you make checks before calling unwrap, and I certainly agree that it's not a critical issue. However, the usage of unwrap in production code is not the best practice, safe unwrapping with ok_or or map_err is more readable and error prone, IMO.

web test, you need to use a talisman or subwallet extension

it seems strange to me that it doesn't work with the default Substrate wallet - Polkadot.js. Is there any specific reason for this? Also, it would be nice to document this as pre-requisite for running e2e tests. Currently, there is only a recommendation to use subwallet.

[0xLucca]

Hey @dastansam

There is another issue with running unit tests, though. Am I missing some step before running tests?

Could you please provide more detailed information about the issue you're encountering?

[keeganquigley]

For me, I'm getting an error when trying to build the Docker image. I get this on both macOS and Ubuntu 20.04.

radioactivedrummer@instance-1:~/openPayroll$ docker build -t open-payroll:0.1.0 .
[+] Building 39.4s (6/6) FINISHED                                                               docker:default
 => [internal] load .dockerignore                                                                         0.1s
 => => transferring context: 2B                                                                           0.0s
 => [internal] load build definition from Dockerfile                                                      0.1s
 => => transferring dockerfile: 290B                                                                      0.0s
 => [internal] load metadata for docker.io/library/rust:1.71                                              0.9s
 => [1/3] FROM docker.io/library/rust:1.71@sha256:92d62e4b8d8a8fd3d59d89b5227d7f7718a6c126990e3c2eac035  35.4s
 => => resolve docker.io/library/rust:1.71@sha256:92d62e4b8d8a8fd3d59d89b5227d7f7718a6c126990e3c2eac0351  0.0s
 => => sha256:92d62e4b8d8a8fd3d59d89b5227d7f7718a6c126990e3c2eac03517f72e5a683 988B / 988B                0.0s
 => => sha256:7ec9143807d4ab7e6f9953fa405e611669a1bfd5faeb866dbacecdcc297f16ed 1.38kB / 1.38kB            0.0s
 => => sha256:58cf5e3f8287db8ea6eb105052e69462dc9505849a2c6b603f25797c270b1ac2 6.09kB / 6.09kB            0.0s
 => => sha256:9a9e034800a1747ea288f38f6087c036acac99dd3ec5255bf7798abd8c23a304 55.06MB / 55.06MB          1.4s
 => => sha256:fd91c1fad56dc2c387d016dbcaaecfbfb5e2b479f8dd644803dead02566f3479 15.76MB / 15.76MB          0.5s
 => => sha256:9dd6a2c0cfa3eb4e8012b3f2df526acc8711a4a9515e0e968419a8ab1e9899e4 54.59MB / 54.59MB          1.0s
 => => sha256:83d4a6da525a13b128e2e4f41081a5af59d2dda8045d40b207510cc3fdc84e1a 196.85MB / 196.85MB        4.2s
 => => sha256:191b9033cd145df313bfeb2cdb96de58b26834e9da12d64c274756fa0f5d4eaa 186.97MB / 186.97MB        5.4s
 => => extracting sha256:9a9e034800a1747ea288f38f6087c036acac99dd3ec5255bf7798abd8c23a304                 5.2s
 => => extracting sha256:fd91c1fad56dc2c387d016dbcaaecfbfb5e2b479f8dd644803dead02566f3479                 0.7s
 => => extracting sha256:9dd6a2c0cfa3eb4e8012b3f2df526acc8711a4a9515e0e968419a8ab1e9899e4                 4.2s
 => => extracting sha256:83d4a6da525a13b128e2e4f41081a5af59d2dda8045d40b207510cc3fdc84e1a                12.1s
 => => extracting sha256:191b9033cd145df313bfeb2cdb96de58b26834e9da12d64c274756fa0f5d4eaa                10.9s
 => [2/3] WORKDIR /src                                                                                    0.6s
 => ERROR [3/3] RUN rustup component add rust-src     && cargo install --force --version 3.1.0 cargo-con  2.3s
------                                                                                                         
 > [3/3] RUN rustup component add rust-src     && cargo install --force --version 3.1.0 cargo-contract:        
0.818 info: downloading component 'rust-src'                                                                   
1.050 info: installing component 'rust-src'                                                                    
1.808     Updating crates.io index
2.234 error: cannot install package `cargo-contract`, it has been yanked from registry `crates-io`
------
Dockerfile:7
--------------------
   6 |     # Update Rust and install cargo contract
   7 | >>> RUN rustup component add rust-src \
   8 | >>>     && cargo install --force --version 3.1.0 cargo-contract
   9 |     
--------------------
ERROR: failed to solve: process "/bin/sh -c rustup component add rust-src     && cargo install --force --version 3.1.0 cargo-contract" did not complete successfully: exit code: 101

[dastansam]

hey @0xLucca,

here it is (unit tests section of Logs)

[0xLucca]

Hey @dastansam @keeganquigley !

We encountered an error with the current version of cargo contract (3.1.0) as it has been removed from crates.io. This led to build failures when trying to install it.

We updated the Docker image with a different version of Rust and cargo contract, which should resolve the yanked version problem.

To test again, please make sure to pull the latest version of the repository to get the updated Dockerfile. When building the image, use the --no-cache flag to ensure you get the latest changes from the repository without using any cached layers. Before running the build command, make sure to delete the target folder under the src directory if it exists.

# Delete the target folder (if it exists)
rm -rf src/target

# Build the image with --no-cache flag
docker build --no-cache -t open-payroll:0.1.0 .

We appreciate your understanding as the encountered issue and necessary changes were beyond our control, resulting from the removal of cargo contract 3.1.0 from crates.io. If you have any questions or need further assistance, feel free to reach out. Thank you once again for your collaboration and support!

[dastansam]

hey @0xLucca,

yes, I was able to run unit tests after pulling new changes. Since all the issues are resolved now, the delivery lgtm. I changed the status to Accepted

cc @keeganquigley

[keeganquigley]

Thanks @dastansam much appreciated. I will forward your payment.