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

QSTN - Deliverables #1083

Merged
merged 9 commits into from
Jan 19, 2024
Merged

QSTN - Deliverables #1083

merged 9 commits into from
Jan 19, 2024

Conversation

qstnus
Copy link
Contributor

@qstnus qstnus commented Dec 16, 2023

Milestone Delivery Checklist

  • The milestone-delivery-template.md has been copied and updated.
  • 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, an invoice must be submitted and the payment will be transferred to the Polkadot/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#1082 < please fill this in with the PR number of your application.

@keeganquigley keeganquigley self-assigned this Dec 20, 2023
Copy link
Contributor

@keeganquigley keeganquigley left a comment

Choose a reason for hiding this comment

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

Thanks for the delivery @qstnus I have the following initial comments:

  • The application in the above description says "XXX", could you replace this with your pull request number? Which is QSTN-Redefining-data-De-Fi-and-NFTs-on-Substrate.md Grants-Program#1082
  • Can you please check off the rest of the boxes as well?
  • Deliverables 4, 5, and 6 are missing from the delivery. Did you complete these and if so can you add them?
  • The milestone tables you copied from the application don't contain any links. Can you please add a link to each section to show us what to evaluate? Here is an example.
  • Also the Docker file that you linked to doesn't exist. I'm not finding it, could you please replace with the correct link?

@keeganquigley
Copy link
Contributor

One pallet unit test is also failing if you could take a look:

test tests::create_new_survey_success ... ok
test tests::create_and_fund_survey_success ... ok
test tests::create_new_survey_fail_already_existing ... ok
test tests::fund_survey_fails_funding_inferior_participants_limit ... ok
test tests::fund_survey_fails_survey_not_created ... ok
test tests::fund_survey_fails_survey_already_funded ... ok
test tests::fund_survey_fails_survey_not_enough_balance ... ok
test tests::fund_survey_success ... ok
test tests::fund_survey_gives_expected_reward_amount_10000_for_1000 ... ok
test tests::fund_survey_fails_survey_not_owner ... ok
test tests::register_participant_fails_max_number_participants_reached ... ok
test tests::register_participant_fails_not_owner ... ok
test tests::register_participant_fails_participant_already_registered ... ok
test tests::register_participant_fails_survey_not_created ... ok
test tests::register_participant_fails_survey_is_not_active ... ok
test tests::register_participant_success ... ok
test tests::register_participant_fails_survey_not_funded ... ok
test tests::reward_participant_fails_already_rewarded ... FAILED
test tests::reward_participant_fails_participant_not_registered ... ok
test tests::reward_participant_fails_not_owner ... ok
test tests::reward_participant_fails_survey_not_created ... ok
test tests::reward_participant_fails_survey_not_funded ... ok
test tests::reward_participant_success ... ok

failures:

---- tests::reward_participant_fails_already_rewarded stdout ----
thread 'tests::reward_participant_fails_already_rewarded' panicked at pallets/survey/src/tests.rs:725:9:
Expected Ok(_). Got Err(
    Module(
        ModuleError {
            index: 2,
            error: [
                10,
                0,
                0,
                0,
            ],
            message: Some(
                "NotOwnerOfSurvey",
            ),
        },
    ),
)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    tests::reward_participant_fails_already_rewarded

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

error: test failed, to rerun pass `-p pallet-survey --lib`

Copy link
Contributor Author

@qstnus qstnus left a comment

Choose a reason for hiding this comment

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

Hello @keeganquigley,

Thanks for this information. Below are answers to your questions. Looking forward to next steps!

  1. I updated the description with our number which is 1082
  2. I checked off the rest of the boxes
  3. I added deliveries 4,5 and 6 to the document
  4. I added a section for links to confirm each delivery
  5. Docker file: https://github.com/QSTN-US/Polkadot-QSTN-v1/tree/main/qstn-substrate-node/substrate-node/docker

In regard to the error message. Our CTO realized there was a logic error where we had to:

  • change line 726 - from "RuntimeOrigin::signed(participant_id)," to "RuntimeOrigin::signed(survey_owner),"

This change allows anyone with an account to withdraw as a survey participant. Previously, only the survey issuer could complete the survey. We fixed this issue and also included a screenshot to show it has been resolved:

photo_2023-12-22_08-54-01

@qstnus
Copy link
Contributor Author

qstnus commented Dec 28, 2023

Hello @keeganquigley,

Thanks for this information. Below are answers to your questions. Looking forward to next steps!

  1. I updated the description with our number which is 1082
  2. I checked off the rest of the boxes
  3. I added deliveries 4,5 and 6 to the document
  4. I added a section for links to confirm each delivery
  5. Docker file: https://github.com/QSTN-US/Polkadot-QSTN-v1/tree/main/qstn-substrate-node/substrate-node/docker

In regard to the error message. Our CTO realized there was a logic error where we had to:

  • change line 726 - from "RuntimeOrigin::signed(participant_id)," to "RuntimeOrigin::signed(survey_owner),"

This change allows anyone with an account to withdraw as a survey participant. Previously, only the survey issuer could complete the survey. We fixed this issue and also included a screenshot to show it has been resolved:

photo_2023-12-22_08-54-01

@semuelle
Copy link
Member

semuelle commented Jan 2, 2024

Thanks for the update, @qstnus. @keeganquigley is still out of office this week, so it'll probably take another week before you'll get some feedback. Sorry about that.

@qstnus
Copy link
Contributor Author

qstnus commented Jan 2, 2024

Hello @semuelle,

Happy New Year! Thanks for the update and we look forward to hearing from you!

Copy link
Contributor

@keeganquigley keeganquigley left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @qstnus a couple of additional requests:

  • The link to the license file is broken, and I'm not finding one in the repo. Can you add the license file back in?
  • I'm not seeing any benchmarking tests for the pallet. Since this milestone would complete the grant, are you willing to either write them, or explain why benchmarking setup is not necessary? Thanks!

@keeganquigley
Copy link
Contributor

keeganquigley commented Jan 10, 2024

Additionally, Docker also fails to build the node on both Ubuntu and Mac. Could you check to see what the issue might be?

Output
./build.sh
~/Polkadot-QSTN-v1/qstn-substrate-node/substrate-node/docker ~/Polkadot-QSTN-v1/qstn-substrate-node/substrate-node/docker
Building QSTN-US/qstn-substrate-node-v010:latest docker image, hang on!
[+] Building 73.3s (10/15)
 => [internal] load .dockerignore                                                                                    0.0s
 => => transferring context: 2B                                                                                      0.0s
 => [internal] load build definition from qstnsubstrate.Dockerfile                                                   0.0s
 => => transferring dockerfile: 1.79kB                                                                               0.0s
 => [internal] load metadata for docker.io/library/ubuntu:20.04                                                      1.3s
 => [internal] load metadata for docker.io/paritytech/ci-linux:production                                            1.2s
 => [builder 1/4] FROM docker.io/paritytech/ci-linux:production@sha256:dc9abf9e877c5bad94828245406dac8a186530e1ad6  56.4s
 => => resolve docker.io/paritytech/ci-linux:production@sha256:dc9abf9e877c5bad94828245406dac8a186530e1ad6a1b5f2072  0.0s
 => => sha256:9d21b12d5fab9ab82969054d72411ce627c209257df64b6057016c981e163c30 31.42MB / 31.42MB                     3.1s
 => => sha256:94ec252bb138516408d499a54bd40801d1c76e3e0182571a90bb138e86cce7e3 588.46MB / 588.46MB                  36.5s
 => => sha256:935ba62d5fdff39246cf9fca5697ae756bfdc34b2957c935254f447f1a296165 409.86MB / 409.86MB                  30.3s
 => => sha256:dc9abf9e877c5bad94828245406dac8a186530e1ad6a1b5f2072e5e19e1f64b4 762B / 762B                           0.0s
 => => sha256:2d60a4916bc3faa064d298c966fd315dd26cd42fd54ec492ec238a99f649ca50 10.01kB / 10.01kB                     0.0s
 => => extracting sha256:9d21b12d5fab9ab82969054d72411ce627c209257df64b6057016c981e163c30                            1.1s
 => => extracting sha256:94ec252bb138516408d499a54bd40801d1c76e3e0182571a90bb138e86cce7e3                           12.3s
 => => extracting sha256:935ba62d5fdff39246cf9fca5697ae756bfdc34b2957c935254f447f1a296165                            7.3s
 => [internal] load build context                                                                                    0.1s
 => => transferring context: 2.62MB                                                                                  0.0s
 => [stage-1 1/6] FROM docker.io/library/ubuntu:20.04@sha256:f2034e7195f61334e6caff6ecf2e965f92d11e888309065da85ff  10.1s
 => => resolve docker.io/library/ubuntu:20.04@sha256:f2034e7195f61334e6caff6ecf2e965f92d11e888309065da85ff50c617732  0.0s
 => => sha256:f2034e7195f61334e6caff6ecf2e965f92d11e888309065da85ff50c617732b8 1.13kB / 1.13kB                       0.0s
 => => sha256:080169816683e6f063d3903434565624287828ecfd06bd2f813b30325e8b1eca 424B / 424B                           0.0s
 => => sha256:fde9c12d7d3f936f56d545cc36391de434bbe311fd9d60f98e496c527cf58f21 2.32kB / 2.32kB                       0.0s
 => => sha256:d519a3a2a796a075e4e40e5c4a1513aa8db8f8fdf009662bf6858f0149143b28 25.97MB / 25.97MB                     9.3s
 => => extracting sha256:d519a3a2a796a075e4e40e5c4a1513aa8db8f8fdf009662bf6858f0149143b28                            0.7s
 => [builder 2/4] WORKDIR /substrate                                                                                 1.3s
 => [builder 3/4] COPY . /substrate                                                                                  0.0s
 => ERROR [builder 4/4] RUN cargo build --release                                                                   14.3s
------
 > [builder 4/4] RUN cargo build --release:
#6 0.383 info: syncing channel updates for 'nightly-x86_64-unknown-linux-gnu'
#6 0.929 info: latest update on 2024-01-10, rust version 1.77.0-nightly (190f4c961 2024-01-09)
#6 0.961 info: downloading component 'cargo'
#6 1.413 info: downloading component 'clippy'
#6 1.874 info: downloading component 'rust-analyzer'
#6 2.221 info: downloading component 'rust-src'
#6 2.387 info: downloading component 'rust-std' for 'wasm32-unknown-unknown'
#6 3.656 info: downloading component 'rust-std'
#6 5.074 info: downloading component 'rustc'
#6 8.589 info: downloading component 'rustc-dev'
#6 13.85 info: downloading component 'rustfmt'
#6 14.03 info: removing previous version of component 'rust-std' for 'wasm32-unknown-unknown'
#6 14.24 info: rolling back changes
#6 14.24 error: could not rename component file from '/usr/local/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib' to '/usr/local/rustup/tmp/m72rs11u31den_bz_dir/bk'
#6 14.24
#6 14.24 Caused by:
#6 14.24     Invalid cross-device link (os error 18)
------
process "/bin/sh -c cargo build --release" did not complete successfully: exit code: 1

Lastly, the polkadot.qstnus.com page also still displays "powered by Near". That makes sense for your regular landing page, but for the Polkadot version it would be nice if this could be changed :)

@qstnus
Copy link
Contributor Author

qstnus commented Jan 18, 2024

Hello @keeganquigley,

Happy New Year and great to hear from you! Below are the answers to your questions. Looking forward to next steps!

  1. We replaced the license file and added in this folder:
    https://github.com/QSTN-US/Polkadot-QSTN-v1/blob/main/LICENSE

  2. We created benchmark testing which can be found here:
    https://github.com/QSTN-US/Polkadot-QSTN-v1/blob/main/qstn-substrate-pallet/survey/src/benchmarking.rs
    https://github.com/QSTN-US/Polkadot-QSTN-v1/blob/main/qstn-substrate-node/substrate-node/node/src/benchmarking.rs

  3. The docker issue has been fixed with evidence below:
    https://github.com/QSTN-US/Polkadot-QSTN-v1/blob/main/qstn-substrate-node/substrate-node/Containerfile

photo_2024-01-18_07-56-51

photo_2024-01-18_07-56-56 (2)

  1. We changed the tagline to "Powered by Polkadot" with evidence below:

Dark mode -
photo_2024-01-18_10-16-01

Light mode -
photo_2024-01-18_10-20-55

@qstnus
Copy link
Contributor Author

qstnus commented Jan 18, 2024

Hello @keeganquigley,

In addition, here is a demo video of the entire project: https://youtu.be/L5tZQTLslFo

@keeganquigley
Copy link
Contributor

Thanks for the updates and for adding the benchmarking tests @qstnus looks good now! I'm happy to pass the milestone, here is my final evaluation. Congrats on completing the grant!

@keeganquigley keeganquigley merged commit 5334c57 into w3f:master Jan 19, 2024
3 checks passed
Copy link

🪙 Please fill out the invoice form in order to initiate the payment process. Thank you!

@qstnus
Copy link
Contributor Author

qstnus commented Jan 19, 2024

Hello @keeganquigley,

Thanks for the final review on our milestone. We look forward to continuing to build within the Polkadot ecosystem.

As a follow up, where should I email my invoice? I have read the documentation, here, and currently in the midst of completing the forms. Unfortunately, I need an email address to finish sending the invoice. Please advise.

@keeganquigley
Copy link
Contributor

Hi @qstnus you don't have to email the invoice, but rather upload it on the Google Form. That way it can be approved and sent to our operations team. Alternatively, if you are having an issue, you can email it to [email protected] and we'll take care of it. Hope that helps!

@qstnus
Copy link
Contributor Author

qstnus commented Jan 19, 2024

Hello @keeganquigley,

Thanks for this information. I sent the invoice following the outlined steps. Please let me know if you have any questions!

@RouvenP
Copy link

RouvenP commented Jan 26, 2024

hi @qstnus we just sent the payment

@qstnus
Copy link
Contributor Author

qstnus commented Jan 26, 2024

Hello @RouvenP,

The payment has been received. Thank you!

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