-
Notifications
You must be signed in to change notification settings - Fork 103
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
docs(x/ecocredit): update ecocredit spec #633
Conversation
Codecov Report
@@ Coverage Diff @@
## master #633 +/- ##
=======================================
Coverage 74.74% 74.74%
=======================================
Files 118 118
Lines 19542 19542
=======================================
Hits 14606 14606
Misses 3963 3963
Partials 973 973
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
Don't we also wanna add project related docs?
Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>
I can add project related docs. The pull request was not merged at the time but it is now. |
I actually think it might be best to follow up with project updates in a separate pull request. The original issue was to update the docs with marketplace functionality. I started making the updates but this pull request is quite large and I'm finding a few other items related to projects that need to be fixed (comments in proto files and state table prefixes). Would love to get this pull request through rather than adding more to it and then address project updates separately if that's ok. |
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.
Makes sense, let's tackle project docs update in a separate PR, is there already an issue for this?
Opened #699. |
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.
Thanks for this great spec update.
Maybe we can change the language a bit for the acceptance tests. Instead:
If a user ....
to
User can / can't ...
If / else formula is already in given - when - then structure.
x/ecocredit/spec/06_tests.md
Outdated
- WHEN - user tries to add an ask denom | ||
- THEN - transaction fails, ask denom is NOT added | ||
|
||
If a user tries to add an ask denom and the user submits a transaction directly from their account, then the transaction fails and the ask denom is NOT added. |
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.
Let's use more simple language:
If a user tries to add an ask denom and the user submits a transaction directly from their account, then the transaction fails and the ask denom is NOT added. | |
User can't add a new _ask denom_ directly, without creating (or linking... let's be more precise here) a gov proposal. |
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.
I don't think changing the language is necessary. I think we should follow the standard format for acceptance tests: https://openclassrooms.com/en/courses/4544611-write-agile-documentation-user-stories-acceptance-tests/4810081-write-acceptance-tests
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.
if-then .... is just another way of saying the same thing using "given - when - then".
The article says about the benefit of using "given - when -then" compared to other format, and it mentions the "if - else" for an exercise:
Let's look at the first example from above:
If the customer tries to withdraw $20 but her account balance is under $20, then do not allow the withdrawal.
Writing that in Given-When-Then format will look like this:
...
Excited to see these spec test cases. I wonder if we could make them executable with some go version of cucumber |
Opened #711 to further discuss and track updates to format and execution. |
|
||
- GIVEN - user provides a sell order id that does exist | ||
- WHEN - user tries to update a sell order | ||
- THEN - transaction is successful, sell order is updated |
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.
I think user should be able to update only his order.
Description
Closes: #604
Update ecocredit specification to include marketplace functionality.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
docs:
prefix in the PR titleReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
docs:
prefix in the PR title