Skip to content
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

Merged
merged 15 commits into from
Jan 28, 2022
Merged

Conversation

ryanchristo
Copy link
Member

@ryanchristo ryanchristo commented Nov 13, 2021

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...

  • included the correct docs: prefix in the PR title
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the documentation writing guidelines
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers 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...

  • confirmed the correct docs: prefix in the PR title
  • confirmed all author checklist items have been addressed
  • confirmed that this PR only changes documentation
  • reviewed content for consistency
  • reviewed content for thoroughness
  • reviewed content for spelling and grammar
  • tested instructions (if applicable)

@ryanchristo ryanchristo changed the title docs: update ecocredit spec docs(x/ecocredit): update ecocredit spec Nov 13, 2021
@ryanchristo ryanchristo added the Scope: x/ecocredit Issues that update the x/ecocredit module label Nov 13, 2021
@codecov
Copy link

codecov bot commented Nov 13, 2021

Codecov Report

Merging #633 (97f6853) into master (63eb64d) will not change coverage.
The diff coverage is n/a.

❗ Current head 97f6853 differs from pull request most recent head 0a308e0. Consider uploading reports for the commit 0a308e0 to get more accurate results

@@           Coverage Diff           @@
##           master     #633   +/-   ##
=======================================
  Coverage   74.74%   74.74%           
=======================================
  Files         118      118           
  Lines       19542    19542           
=======================================
  Hits        14606    14606           
  Misses       3963     3963           
  Partials      973      973           
Flag Coverage Δ
experimental-codecov 74.84% <ø> (ø)
stable-codecov 67.65% <ø> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@ryanchristo ryanchristo marked this pull request as ready for review December 9, 2021 22:52
Copy link
Member

@blushi blushi left a 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?

x/ecocredit/spec/03_messages.md Outdated Show resolved Hide resolved
x/ecocredit/spec/03_messages.md Outdated Show resolved Hide resolved
x/ecocredit/spec/03_messages.md Outdated Show resolved Hide resolved
ryanchristo and others added 2 commits January 20, 2022 13:44
Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>
@ryanchristo
Copy link
Member Author

Don't we also wanna add project related docs?

I can add project related docs. The pull request was not merged at the time but it is now.

@ryanchristo
Copy link
Member Author

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.

Copy link
Member

@blushi blushi left a 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?

@ryanchristo
Copy link
Member Author

Makes sense, let's tackle project docs update in a separate PR, is there already an issue for this?

Opened #699.

Copy link
Collaborator

@robert-zaremba robert-zaremba left a 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.

- 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.
Copy link
Collaborator

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:

Suggested change
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.

Copy link
Member Author

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

Copy link
Collaborator

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:
...

@ryanchristo ryanchristo mentioned this pull request Jan 26, 2022
13 tasks
@aaronc
Copy link
Member

aaronc commented Jan 26, 2022

Excited to see these spec test cases. I wonder if we could make them executable with some go version of cucumber

@aaronc
Copy link
Member

aaronc commented Jan 26, 2022

@ryanchristo
Copy link
Member Author

Opened #711 to further discuss and track updates to format and execution.

@ryanchristo ryanchristo enabled auto-merge (squash) January 28, 2022 20:03
auto-merge was automatically disabled January 28, 2022 20:03

Pull Request is not mergeable

@ryanchristo ryanchristo enabled auto-merge (squash) January 28, 2022 20:04
@ryanchristo ryanchristo merged commit a56674c into master Jan 28, 2022
@ryanchristo ryanchristo deleted the ryan/604-update-spec branch January 28, 2022 20:05

- 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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: x/ecocredit Issues that update the x/ecocredit module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update ecocredit specification with marketplace functionality
6 participants