Grant-Milestone-Delivery icon indicating copy to clipboard operation
Grant-Milestone-Delivery copied to clipboard

Create Rubeus_keeper_Milestone_1.md

Open dsrdrk11a opened this issue 2 years ago • 1 comments

Milestone Delivery Checklist

Link to the application pull request: https://github.com/w3f/Grants-Program/pull/1124

dsrdrk11a avatar Nov 03 '22 18:11 dsrdrk11a

Just uploaded a new invoice. Sorry, the first one was incorrect.

dsrdrk11a avatar Nov 08 '22 19:11 dsrdrk11a

@keeganquigley , hi! Could you, please, give a feedback on MS1? We've already finished the second milestone and ready to deliver, but need to commit changes to the rep that you might observe.

dsrdrk11a avatar Nov 17 '22 17:11 dsrdrk11a

Hi @dsrdrk11a I'm terribly sorry for the delay, the grants team has been a bit behind as of late and is finally catching up.

  1. Regarding your second milestone, you can work on them concurrently, you don't have to wait for me to finish this one. I went ahead and forked your repo to our W3F grants archive so I can work from there and you can continue to make changes. Another alternative that teams often do is create a separate branch for each milestone.

  2. I was able to get all the way through the guide until it came to deploying the smart contract on substrate-contracts-node. When running a v0.22.1 node I get this error during the upload and deploy step: error

I am deploying as Alice with 200,000 gas:

max gas

I tried deploying on an older version of substrate-contracts-node v0.16.0 but it gives a code rejected error:

instantiate error

I did have to use cargo +stable contract build to build it since it is now possible to build contracts using the +stable toolchain. Is it possible that you have some unstable function(s) that isn't supported by the regular contracts node? I found this issue for reference. Any help would be greatly appreciated!

I am using:

rustc --version
rustc 1.67.0-nightly (e9493d63c 2022-11-16)

cargo --version
cargo 1.67.0-nightly (16b097879 2022-11-14)

cargo contract --version
cargo-contract 2.0.0-alpha.3-0e4a286-aarch64-apple-darwin

installed toolchains
--------------------

stable-aarch64-apple-darwin
nightly-2022-02-20-aarch64-apple-darwin
nightly-2022-03-14-aarch64-apple-darwin
nightly-2022-06-20-aarch64-apple-darwin
nightly-2022-06-30-aarch64-apple-darwin
nightly-aarch64-apple-darwin (default)

installed targets for active toolchain
--------------------------------------

aarch64-apple-darwin
wasm32-unknown-unknown

active toolchain
----------------

nightly-aarch64-apple-darwin (default)
rustc 1.67.0-nightly (e9493d63c 2022-11-16)

keeganquigley avatar Nov 17 '22 22:11 keeganquigley

@keeganquigley the problem is because substrate-contract-node version 0.22.1 is not yet officially worked with cargo-contract. This is stated in the release notes. https://github.com/paritytech/substrate-contracts-node/releases/tag/v0.22.1

Currently, cargo-contract and ContractsUI do not work with this release as they need to be adapted: Use version 0.21 on which everything works.

Regarding the compiler - in rust there is a fairly common problem of regressions in the nightly builds of the compiler, I can not answer why the compiler does not build this contract. In past projects that were written under ink! when a nightly build of the compiler was still required, there was a common problem that it was necessary to select compiler versions for a specific build day. Now that the features are stabilized, it's better to use the predictable stable version 1.65.

friktor avatar Nov 21 '22 13:11 friktor

Thanks @friktor, no worries about the compilation error for this milestone, as I know it's an issue with ink! dependencies and not the project. I was able to compile it and run it on the 0.21.0 node as you suggested. I also switched the stable 1.65 version. Please see points below:

  1. Your smart contract deliverable states you will build the following functions: addPassword , getPassword, deletePassword. I'm assuming that these are synonymous with the delivered ones: addCredential, deleteCredential, getCredential but where are the addCategory, getCategory, deleteCategory functions? Are they no longer necessary? Are you still using the same encrypted streaming algo?

  2. Most of the functions are missing documentation:

no docs provided

  1. Much of the code is also missing documentation. For example it's hard to tell what the E2E tests are doing since most of the functions don't have any inline comments.

  2. Your video is very helpful thank you! However, it stops after the contract is built. I was hoping it would go further to show me how to make the contract calls. You give some example usage, but it would be nice if you could add to the steps to include an example scenario of how to upload a payload, transfer ownership, etc.

  3. I'm curious about the design of your contract unit tests: The contract only contains one unit test check_crud_credential that includes all functions. Typically the conventional method is to create a separate unit test for each function/method. One for get_credential, one for delete_credential, one for transfer_ownership, etc. That way if a component breaks you can tell which one is failing, instead of the whole test crashing. Is there a reason you designed it to include everything in one test?

Let me know if you can address the issues above. Thanks!

keeganquigley avatar Nov 22 '22 21:11 keeganquigley

Thanks, @keeganquigley for your detailed reply. When developing the solution we have come to the conclusion, that making separate methods for categories will only add UX complexity and additional transactional costs, which can be notable in such a service, that should be maximum cheap. So we didn't exclude the essence of categories, but added group parameter in credentials list (and also renamed Passwords to Credentials). You can make selection by this parameter with getCredentialsByGroup. For the encryption algo: yes, we still use chacha20, but for security reasons encryption is executed on the front, so smart contract receives already encrypted data. You can check how it works here and here

All your comments about code documetation we've taken into account and will correct the code and videos. I'll write you when done.

dsrdrk11a avatar Nov 23 '22 18:11 dsrdrk11a

Hey, @keeganquigley, we've made corrections and added more detailed descriptions, please check.

About your 3rd remark: this is rather a very simple example of using a contract along with client-side encryption, we added more detailed comments about the encryption mechanism and also renamed the code file to example.ts to make it clear.

  1. There's also a second video showing the usage of the test page, please chack, seems that you've missed it. Or if you want to test the "raw" mechanics you can run the example from the example folder (but first run substrate-contracts-node)

  2. Now there are 5 tests: - create_credential, transfer_ownership, delete_credential, update_credential, list_of_credentials.

dsrdrk11a avatar Nov 24 '22 19:11 dsrdrk11a

Hi, @keeganquigley do you have more questions about the 1st MS? I've just made the 2nd MS delivery and it is added to the same pull request, as the first one was not approved still. Please, check.

dsrdrk11a avatar Dec 01 '22 14:12 dsrdrk11a

thanks for making the changes @dsrdrk11a and apologies for the delay, I had vacation and then a conference back-to-back. I am back in the office now so I will look at this and get back to you tomorrow.

keeganquigley avatar Dec 01 '22 16:12 keeganquigley

Thanks again @dsrdrk11a you can find my evaluation here. All above points were addressed. The only outstanding issues are under the Smart Contract section. Can you please take a look at these?

Also, can you please remove M2 from this delivery and submit it as a separate PR? That way I can merge this one in order to get you paid for M1. Otherwise you would need to wait until M2 evaluation is complete to get paid. Thanks!

keeganquigley avatar Dec 05 '22 15:12 keeganquigley

Thanks, @keeganquigley ! I've deleted the MS2 file, you can merge.

dsrdrk11a avatar Dec 05 '22 16:12 dsrdrk11a

Thanks @dsrdrk11a much appreciated! I assume you will submit M2 as a separate PR shortly. I am happy to accept M1 and will forward your invoice for payment. Please take a look at the issues I mentioned above to fix for M2. I will start the eval now!

keeganquigley avatar Dec 05 '22 17:12 keeganquigley

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 [email protected] 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.

github-actions[bot] avatar Dec 05 '22 17:12 github-actions[bot]

@keeganquigley that is great! I will pull MS2 shortly, just check your comments.

dsrdrk11a avatar Dec 05 '22 17:12 dsrdrk11a

hi @dsrdrk11a we're currently switching from USDT on Ethereum to Polkadot/Kusama. Would you be open to receive USDT on Polkadot(Statemint) or Kusama(Statemine)?

RouvenP avatar Dec 07 '22 08:12 RouvenP

Hi, @RouvenP Can we proceed with Eth address on this milestone and switch to Polkadot on the next one? Just not to jump off the deep end

dsrdrk11a avatar Dec 07 '22 10:12 dsrdrk11a

hi @dsrdrk11a absolutely, we will proceed with ETH-USDT. Thanks for the feedback

RouvenP avatar Dec 09 '22 08:12 RouvenP

hi @dsrdrk11a we transferred the payment today.

RouvenP avatar Dec 16 '22 12:12 RouvenP