External evaluation of Dotflow Milestone 1

[dastansam]

External evaluation for the Dotflow Milestone 1

• Delivery PR
• Application [PR] (Dotflow Grants-Program#1657)

[Szegoo]

@dastansam Thanks a lot for your evaluation! I have added instructions regarding contract deployment and frontend environment variable.

I have also resolved all the clippy errors, thanks for pointing that out.

Regarding e2e-tests, yes we were planning to add them once we have all the pieces of the logic.

The reason why we use assert or unwrap in some parts of the code is because we have ensured earlier that the unwrap will be successful(like here).
Also, using assert and unwrap in the constructor is in my opinion ok since it is not a problem if the code panics during the contract deployment, but if that is a problem we can update the code.

[dastansam]

hey @Szegoo,

thanks for the swift feedback.

The reason why we use assert or unwrap in some parts of the code is because we have ensured earlier that the unwrap will be successful(like here).

yeah, I went through it again and you are right, unwrap calls are safe. I only have one suggestion for a clearer syntax, then:

// instead of this
ensure!(self.number_to_identity.get(receiver).is_some(), Error::IdentityDoesntExist);

let receiver_identity = self.number_to_identity.get(receiver).unwrap();

// do this: it's one line and one storage read less :)
let receiver_identity = self.number_to_identity.get().map_or(Error::IdentityDoesntExist, |v| Ok(v))?;

Also, using assert and unwrap in the constructor is in my opinion ok since it is not a problem if the code panics during the contract deployment, but if that is a problem we can update the code.

I think in general, it's not that critical to use unwrap and assert in the contract env, compared to runtime or client env, where it is unacceptable. But it's still better to avoid it, IMO.

But overall, again, I don't see any critical issues that needs to be addressed.

UPDATE: updated the code to use map_or instead of map_err, since get() returns an Option

[Szegoo]

@dastansam Yes, you are right. It is a good idea to refactor the code the way you described. Thanks for your suggestion. TheDotflow/dotflow-ink#38

[dastansam]

It's good to go for me, let's wait for @keeganquigley's evaluation

[keeganquigley]

Thanks for the evaluation @dastansam excellent work! @Szegoo I agree, everything looks great and I am able to reproduce the results. A couple of comments:

• I agree that it's nice to have the process of deploying the contract on a local testnet included for those who may not have done it before. Thanks for adding it and improving the docs overall.
• I agree, no worries about the e2e tests until the next milestone.
• @dastansam could you change the status to accepted? Once that is done I will merge it along with the delivery.

Thanks!

[dastansam]

@keeganquigley changed the status of the evaluation to Accepted

[keeganquigley]

Thanks a bunch @dastansam I also made a minor title fix to adhere to our naming convention. I will forward your KSM address to our operations team to process your payment.

[RouvenP]

hi @dastansam we just transferred the KSM