Skip to content

Milestone Delivery: Escrow pallet Milestone #1#566

Merged
keeganquigley merged 22 commits intow3f:masterfrom
herou:escrow_milestone1
Sep 22, 2022
Merged

Milestone Delivery: Escrow pallet Milestone #1#566
keeganquigley merged 22 commits intow3f:masterfrom
herou:escrow_milestone1

Conversation

@herou
Copy link
Copy Markdown
Contributor

@herou herou commented Sep 9, 2022

Milestone Delivery Checklist

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

@herou herou changed the title Add escrow-milestone_1.md file Milestone Delivery: Escrow pallet Milestone #1 Sep 9, 2022
@takahser
Copy link
Copy Markdown
Contributor

@herou thanks for your delivery! We currently have a rather large backlog to tackle, so it might take a bit longer than usual until your delivery is evaluated. Nevertheless, we're going to take a look at it as soon as we can.

@takahser takahser assigned takahser and unassigned takahser Sep 14, 2022
@keeganquigley
Copy link
Copy Markdown
Contributor

Hi @herou I have started the process of evaluating your milestone. Please give me a bit of time to fully understand your project and I will reach out to you with any questions I may have. Thanks!

@keeganquigley
Copy link
Copy Markdown
Contributor

keeganquigley commented Sep 15, 2022

@herou I looked over your milestone and am getting 404 errors on Deliverable 4. Docker and 6. Escrow Pallet. Can you fix these links? Thanks!

@herou
Copy link
Copy Markdown
Contributor Author

herou commented Sep 15, 2022

@herou I looked over your milestone and am getting 404 errors on Deliverable 4. Docker and 6. Escrow Pallet. Can you fix these links? Thanks!

It should be fine now. I made my repo public.

@keeganquigley
Copy link
Copy Markdown
Contributor

keeganquigley commented Sep 16, 2022

Thanks @herou a couple more things to address:

  1. Can you provide a link to the license file? This will be needed for submission.
  2. Is there a reason the license file is in the master branch and the rest of the pallet is in eljo-prifti/escrow?

@herou
Copy link
Copy Markdown
Contributor Author

herou commented Sep 16, 2022

Thanks @herou a couple more things to address:

  1. Can you provide a link to the license file? This will be needed for submission.
  2. Is there a reason the license file is in the master branch and the rest of the pallet is in eljo-prifti/escrow?

Here it is: https://github.com/herou/EscrowPallet/blob/eljo-prifti/escrow/LICENSE

@herou
Copy link
Copy Markdown
Contributor Author

herou commented Sep 16, 2022

Thanks @herou however I was actually asking if you can update this on the milestone delivery itself. Right now it just says Apache 2.0 but doesn't link to it. Sorry for the confusion there. Can you update this?

I added it to the milestone delivery doc.

@keeganquigley
Copy link
Copy Markdown
Contributor

Thanks @herou moving on, your pallet code looks good and the unit tests are all successful. However, some of the benchmarking tests are failing when running cargo test --features runtime-benchmarks. Are you able to address these 2 errors?

  error[E0425]: cannot find function `assert_last_event` in this scope
    --> /Users/keeganquigley/EscrowPallet/pallets/escrow/src/benchmarking.rs:30:3
     |
  30 |      assert_last_event::<T>(Event::Locked(caller, amount).into());
     |      ^^^^^^^^^^^^^^^^^ not found in this scope

     Compiling frame-system-benchmarking v4.0.0-dev (https://github.com/paritytech/substrate.git?branch=polkadot-v0.9.28#34a06217)
  error[E0599]: no function or associated item named `max_value` found for associated type `<<T as pallet::Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance` in the current scope
    --> /Users/keeganquigley/EscrowPallet/pallets/escrow/src/benchmarking.rs:23:32
     |
  23 |         let amount = BalanceOf::<T>::max_value();
     |                                      ^^^^^^^^^ function or associated item not found in `<<T as pallet::Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance`
     |
     = help: items from traits can only be used if the trait is in scope
  help: the following traits are implemented but not in scope; perhaps add a `use` for one of them:
     |
  3  | use frame_support::sp_runtime::traits::Bounded;
     |
  3  | use num_traits::bounds::UpperBounded;
     |
  3  | use num_traits::float::FloatCore;
     |

  Some errors have detailed explanations: E0425, E0599.
  For more information about an error, try `rustc --explain E0425`.
  warning: `escrow` (lib) generated 2 warnings
  error: could not compile `escrow` due to 2 previous errors; 2 warnings emitted
  warning: build failed, waiting for other jobs to finish...

@herou
Copy link
Copy Markdown
Contributor Author

herou commented Sep 17, 2022

cargo test --features runtime-benchmarks

Sorry for this inconvenience, pushed my fix. Please take another look. @keeganquigley please let me know asap if this is okay because I am ready to push the second milestone. Thank you!

@alxs
Copy link
Copy Markdown
Contributor

alxs commented Sep 20, 2022

@herou we don't require milestones to be delivered sequentially, so feel free to create a PR for the second milestone already.

@keeganquigley
Copy link
Copy Markdown
Contributor

keeganquigley commented Sep 20, 2022

Apologies for the delay @herou and thanks for fixing the benchmarking tests! The fix did work. The pallet looks good and I am finishing up my eval now.

@keeganquigley
Copy link
Copy Markdown
Contributor

keeganquigley commented Sep 21, 2022

@herou apologies for the delay and thanks for your patience during this process. I'm ready to submit my evaluation, which you can find here. The only outstanding issue concerns the unit tests. Please consider adding these for the next milestone.

@herou
Copy link
Copy Markdown
Contributor Author

herou commented Sep 21, 2022

@herou apologies for the delay and thanks for your patience during this process. I'm ready to submit my evaluation, which you can find here. The only outstanding issue concerns the unit tests. Please consider adding these for the next milestone.

Hi, thank you for your reply. NoValueStored and WrongExpiringDate are going to be used in the later milestones. If you see the sign_contract does not throw any of these errors, which means I can not write tests for them. SameAddressError error is used in the sign_contract, that's why I have written a test for it. Is it clear to you? Please, @elio.prifti:matrix.org write me directly on element if it is not clear to you. I would like to thank you for your time and effort. Thanks a lot!

@keeganquigley
Copy link
Copy Markdown
Contributor

@herou okay thanks for clarifying! Yes in that case I am happy to accept this milestone! I have submitted the PR and will let the rest of the team know.

@herou
Copy link
Copy Markdown
Contributor Author

herou commented Sep 21, 2022

@herou okay thanks for clarifying! Yes in that case I am happy to accept this milestone! I have submitted the PR and will let the rest of the team know.

My first milestone accepted :)

@keeganquigley keeganquigley merged commit 47c6f3e into w3f:master Sep 22, 2022
@takahser
Copy link
Copy Markdown
Contributor

@herou could you upload a new invoice that includes your home address, please?
Also, please make sure to include the W3F's address:

Web 3.0 Technologies Foundation
Address: Baarerstrasse 14, 6300 Zug, Switzerland
UID / VAT ID: CHE-322.596.347

@herou
Copy link
Copy Markdown
Contributor Author

herou commented Sep 22, 2022

@herou could you upload a new invoice that includes your home address, please? Also, please make sure to include the W3F's address:

Web 3.0 Technologies Foundation Address: Baarerstrasse 14, 6300 Zug, Switzerland UID / VAT ID: CHE-322.596.347

Uploaded it, please let me know if it is okay. Thank you!

@takahser
Copy link
Copy Markdown
Contributor

@herou thanks. I think, in your address the town and country are still missing though, can you add them and re-upload?

@herou
Copy link
Copy Markdown
Contributor Author

herou commented Sep 22, 2022

@herou thanks. I think, in your address the town and country are still missing though, can you add them and re-upload?

Can you please check it now and let me know? Thank you!

@keeganquigley
Copy link
Copy Markdown
Contributor

@herou it looks good now, thanks!

@herou
Copy link
Copy Markdown
Contributor Author

herou commented Sep 25, 2022

@keeganquigley I really liked your review and I would like to have a short conversation with you if possible. Can you please write me on twitter: https://mobile.twitter.com/eljoprifti1 I would really appreciate it. Looking forward to hearing from you. You rock it!

@keeganquigley
Copy link
Copy Markdown
Contributor

@herou thanks for the compliments, and sure thing! I will DM you now.

@takahser
Copy link
Copy Markdown
Contributor

@herou your address seems to be a tron address which we don't currently support for payouts:

Payment is made in Bitcoin, USDT, DAI or aUSD.

The amount in your grant application is denominated in USDT, which would work for us. Hence, could you please also update the wallet address?

@herou
Copy link
Copy Markdown
Contributor Author

herou commented Sep 27, 2022

@herou your address seems to be a tron address which we don't currently support for payouts:

Payment is made in Bitcoin, USDT, DAI or aUSD.

The amount in your grant application is denominated in USDT, which would work for us. Hence, could you please also update the wallet address?

Can you please check it again?

@keeganquigley
Copy link
Copy Markdown
Contributor

thanks @herou we received the updated invoice but can you update it in the contract as well? Thanks!

@herou
Copy link
Copy Markdown
Contributor Author

herou commented Sep 27, 2022

thanks @herou we received the updated invoice but can you update it in the contract as well? Thanks!

It can not be done because the PR has been merged.

@keeganquigley
Copy link
Copy Markdown
Contributor

@herou ah right, okay I will check with @takahser on how to proceed.

@keeganquigley
Copy link
Copy Markdown
Contributor

@herou I checked with the team, and the contract will need to be amended. You can submit another PR for this. Thanks and apologies for the inconvenience.

@herou
Copy link
Copy Markdown
Contributor Author

herou commented Sep 27, 2022

@herou I checked with the team, and the contract will need to be amended. You can submit another PR for this. Thanks and apologies for the inconvenience.

This is a new PR of changing the address: w3f/Grants-Program#1190

@herou
Copy link
Copy Markdown
Contributor Author

herou commented Oct 4, 2022

@herou I checked with the team, and the contract will need to be amended. You can submit another PR for this. Thanks and apologies for the inconvenience.

This is a new PR of changing the address: w3f/Grants-Program#1190

Hi @keeganquigley . Can you please let me know the status of the payment of this milestone?

@keeganquigley
Copy link
Copy Markdown
Contributor

@herou confirmed that he has received payment for the milestone, noting this here.

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