-
Notifications
You must be signed in to change notification settings - Fork 528
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
Ssal Deliverables #1109
Ssal Deliverables #1109
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @MatteoPerona, thanks for the delivery and sorry for the long wait. It took me a bit to understand the logic of the smart contract, as I'm not so familiar with commodities. Everything looks good so far, I just have a few questions about the functionality in general.
- Since only the buyer can finalize, how do you ensure that the seller actually receives the
total
? Are you planning any kind of (off-chain) dispute mechanism? You includeEDIT: I just noticed that you definedvolume
, but no indication of the unit of measurement. Would each commodity have its own smart contract so that the unit is the same for all commodity contracts and different for each smart contract?volume
as grams. Then the question becomes: aren't there commodities that are measured in other units, e.g. by volume instead of weight?- Have you considered making payments in native tokens? Payments are made in fungible tokens that are created with the smart contract, which adds a lot of friction. In order to buy a contract, I would have to own these specific tokens first, and there is no way to pay with tokens in one smart contract for commodities in another deployment. Also, I don't see any mention of a token in your grant application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MatteoPerona, could you also look at my notes about the tests? Ideally, the unit tests don't just cover the happy paths, and the e2e test failed in my case.
Hey @semuelle, thank you for the great feedback and taking the time to understand my work. Below I quoted all your questions and did my best to give an informative response. At the end of this comment, I left a short conclusion where I discuss what I think makes sense to change now, vs what I would leave for later. Let me know what you think makes sense.
For future iterations, the idea was to implement a sort of on-chain trial system that would decide the fate of the locked balance. I thought this was out of scope for this iteration of the project. Refer to the last point in the future plans section in the proposal.
Yes, I realized that weight made more sense than volume through development, so I changed the type but left the variable name. This was an oversight because I had been using the term volume so much throughout the rest of the project. I'll amend that.
Yes, I originally tried to implement payments with native tokens, but, after lots of trial and error and speaking with an Ink! team member, I decided that a native token works better for the purposes of this contract. I wanted the contract to be deployable to any Substrate based chain, and that seems to currently be impossible while using the native token for transactions since I would have to write chain extensions or depend on that infrastructure being set up for each blockchain. I want people to be able to deploy this anywhere and change it for the better; I think it's more pliable in its current state. I agree, however, that there is a glaring flaw if one deployed another instance of the contract. Perhaps, the best solution would be to implement transaction using smart contract tokens. In this way, if a developer only has to deploy a compatible token along with the ssal smart contract in order to be able to use it.
I looked through your notes and I'm not sure what went wrong when you were running the tests, but I will try to replicate the error and fix it. Additionally I wrote functionality to test unhappy paths in each unit test, but I think I should have separated them into their own tests for more clarity. Also, I've since thought of some cases that need to be tested. Concluding ThoughtsI think going forward the best plan of action is to immediately change every mention of volume to weight in the code, docs, and article. Also, I will revise the tests, separating happy and unhappy paths and fixing the e2e test error. As for dispute resolution and the token functionality / using native token, I think those would work as a great second iteration to the project. What do you think of this plan? Should I change anything else before the next review? |
Hey @MatteoPerona, thanks for the thorough reply. This all sounds very reasonable. The token isn't mentioned in the agreement, so it's really up to you. The tests should be as thorough as possible for the sake of your users. Regarding tokens, yes, using the same token for all smart contracts would make sense and it would probably make it easier to later switch to a native token. I would suggest to implement that right away, but it also depends on what the roadmap for the project looks like beyond the grant. If you were planning to use the smart contracts as they are, feel free to leave it. |
pinging @MatteoPerona |
Hey @semuelle, Apologies for the late reply, I'm heading into a very busy patch. In the grant proposal I don't think I included dispute resolution as a deliverable, but I can do my best to integrate some basic form of decentralized voting in the coming month. I will be very busy until March 15th though, so my next PR might not come until then. As for the tokens, I will leave them as is for the time being. In summary, I'll change the volume variable to weight everywhere it is mentioned, I will revise the tests making them clearer and adding any necessary cases, and I will implement a basic dispute resolution mechanism before the next PR. Does that sound like a good plan to you? Thanks for your continued feedback. |
Don't worry about the dispute resolution, as it's not part of the agreement. If you can look into the tests and possibly the handling of units, I'd be content. |
pinging @MatteoPerona |
Hi @keeganquigley thanks for pinging. Those last few changes should be completed by Monday. Apologies for the drawn out schedule, I'm really overextended at the moment. |
Hey there, I just finished making changes. Should I create a new PR? Just not sure because the changes did not affect any of the functionality: just semantics or test related changes. Recap: Thanks for your patience. Let me know what the next steps are. |
Hey @MatteoPerona. Since this PR is still open, I don't think it's necessary to open another one. I will review the changes in the coming days. Did you document them anywhere? |
Hi @semuelle, I didn't document more than my commits, but all of my changes are as we discussed: "change every mention of volume to weight in the code, docs, and article. Also, I will revise the tests, separating happy and unhappy paths and fixing the e2e test error." The one caveat was that the e2e error was an expected case for an unhappy path. If it helps here's a list of the changes:
|
Pinging @semuelle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, @MatteoPerona, your milestone has been accepted, see my evaluation notes here.
🪙 Please fill out the invoice form in order to initiate the payment process. Thank you! |
We noticed that this is the last milestone of your project. Congratulations on completing your grant! 🎊 |
Milestone Delivery Checklist
Link to the application pull request: w3f/Grants-Program#1986< please fill this in with the PR number of your application.