Skip to content

add deliveries/substrate-tutorials-milestone_1.md#458

Merged
lucasvanmol merged 1 commit intow3f:masterfrom
rusty-crewmates:substrate-tutorials-milestone-1
Aug 22, 2022
Merged

add deliveries/substrate-tutorials-milestone_1.md#458
lucasvanmol merged 1 commit intow3f:masterfrom
rusty-crewmates:substrate-tutorials-milestone-1

Conversation

@tdelabro
Copy link
Copy Markdown
Contributor

Milestone Delivery Checklist

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

@tdelabro tdelabro force-pushed the substrate-tutorials-milestone-1 branch 3 times, most recently from 00c0d81 to 66808ee Compare June 10, 2022 03:44
@tdelabro tdelabro force-pushed the substrate-tutorials-milestone-1 branch from 66808ee to f49505b Compare June 10, 2022 03:45
@lucasvanmol lucasvanmol self-assigned this Jun 10, 2022
@lucasvanmol
Copy link
Copy Markdown
Contributor

Hi @tdelabro, thank you for this delivery! Since I'm new to substrate development I was excited to evaluate this milestone as I'm pretty much the target audience :)

Anyway, here's my in progress evaluation: https://github.com/lucasvanmol/Grant-Milestone-Delivery/blob/4c10ab49cce021d33692e00d6e4795931e90fca7/evaluations/substrate-tutorials_1_lucasvanmol.md

Could you take a look and try to address the issues I mention? Of course, feel free to ask me any questions about the evaluation.

@tdelabro
Copy link
Copy Markdown
Contributor Author

Hey @lucasvanmol,

Thanks for the review. I hope you enjoyed discovering our course.
We will do the required changes and come back to you really soon !

Thanks a lot for your time !

@lucasvanmol
Copy link
Copy Markdown
Contributor

Hey there @tdelabro,

I see there's been some updates on the repo regarding my feedback, and I'm wondering if you feel like it's ready for review or would like more time to work on it? There's no rush, of course, just checking in :)

@tdelabro
Copy link
Copy Markdown
Contributor Author

Hi @lucasvanmol No it's not ready for review yet. We fixed most of the typos now, but we still have some work to do to match your review.

I will notify you soon ;)

@tdelabro
Copy link
Copy Markdown
Contributor Author

tdelabro commented Aug 6, 2022

@lucasvanmol I think we are ready for a new review.
We fixed the spellings, and make most pallets compile even with todo! in place.

Hope you will find it easier to use like this

@lucasvanmol
Copy link
Copy Markdown
Contributor

Thanks for the update! I'm working through the tutorial again and I'm really liking the update made to the writing. However, I've run into some issues with the update that you included to check for events being emitted (which is a great addition). The tests seem to fail to properly check for emitted events: for example the function create inside ex01, which is an example function, does not pass the tests. You can verify by running:

cargo test -p pallet-assets -- create::ok

which runs the ok test for the example create function. The output is an issue with the event:

running 1 test
test tests::assets::create::ok ... FAILED

failures:

---- tests::assets::create::ok stdout ----
thread 'tests::assets::create::ok' panicked at 'Event expected', exercises/ex01-fungible-token/assets/src/tests/assets.rs:7:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    tests::assets::create::ok

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

When trying to fix the issue, I noticed that the solutions branch of the tutorial repo had not been updated to include the new event changes, which is perhaps why this issue was not caught.

Could you please investigate and update the solutions branch in addition to the example function in the main repo? I have also not tested for similair errors in the other exercises (except for ex00, which works fine), so I suspect those might have issues as well.

@tdelabro
Copy link
Copy Markdown
Contributor Author

tdelabro commented Aug 21, 2022

Hey @lucasvanmol,

Glad to hear we got our English and explanations got better.

Sorry for the events, we totally missed it. The reason is that to emit events, the chain must be at least at block 1 (not block 0/genesis), which is not the case by default when building the TestExternalities in our mock.rs files.
I added ext.execute_with(|| System::set_block_number(1)); where it was useful, and now it works.

I also updated the solutions branch (actually I created a new one, based on the main, when I was doing the opposite before. Should be better for code maintenance) with all the new answers.

@lucasvanmol
Copy link
Copy Markdown
Contributor

Thank you for the update @tdelabro , I've run through all the exercises & solutions again and you've made some great improvements to the project! As such I've accepted the delivery and you can read through my final evaluation here: https://github.com/w3f/Grant-Milestone-Delivery/blob/master/evaluations/substrate-tutorials_1_lucasvanmol.md

I'll forward your invoice internally - please allow for up to 2 weeks for processing.

(I only have one a small nitpick: regarding the Mint struct/event used in ex01-fungible-token. The term owner is a bit confusing - I initially thought this referred to the asset owner, not the account the asset was minted to. Perhaps it could be changed to to?)

@lucasvanmol lucasvanmol merged commit 58d59b7 into w3f:master Aug 22, 2022
@github-actions
Copy link
Copy Markdown

Congratulations on completing the first milestone of this grant! As part of the Grants Program, we want to help grant recipients acknowledge their grants publicly. To that end, we’ve created a badge for projects that successfully deliver their first milestone. Note that it must only be used within the context of the delivered work, so please do not display it on your team or project's homepage unless accompanied by a short description of the grant.

Furthermore, you're now welcome to announce the grant publicly. Please remember to observe the foundation’s guidelines in doing so. In case you haven't done so yet, you may also reach out to grantsPR@web3.foundation for feedback on your announcement and cross-promotion.

Thank you for your contribution and good luck with the remaining milestones, if any! As usual, please let us know if you run into any delays by leaving a comment on the application PR, or directly submitting an amendment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants