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

Add docstrings and additional comments #234

Merged
merged 6 commits into from Apr 15, 2019

Conversation

@BrendanChou
Copy link
Contributor

commented Apr 12, 2019

Fixes #199

@BrendanChou BrendanChou requested a review from AntonioJuliano Apr 12, 2019

@BrendanChou BrendanChou force-pushed the bc_docstring branch from 666c41b to b707267 Apr 12, 2019

@BrendanChou BrendanChou force-pushed the bc_docstring branch from b707267 to 11abb2e Apr 12, 2019

Show resolved Hide resolved contracts/protocol/Admin.sol Outdated
Show resolved Hide resolved contracts/protocol/Admin.sol Outdated
*
* @param marketId The market to query
* @return The current market info, including:
* - The ERC20 token address

This comment has been minimized.

Copy link
@AntonioJuliano

AntonioJuliano Apr 12, 2019

Member

since we're using AbiEncoderV2 these will be returned as structs with named paramaters. It would be a lot more helpful to see this as:

{
  keyA: description of keyA,
  keyB: description of keyB
}
*
* @param accounts A list of all accounts that will be used in this operation. Cannot contain
* duplicates. In each action, the relevant account will be referred-to by its
* index in the list.

This comment has been minimized.

Copy link
@AntonioJuliano

AntonioJuliano Apr 12, 2019

Member

it'd be very helpful to see at a glance here the structure of an Account.Info. Also of the form:

{
  keyA: descA,
  keyB: descB
}

Same for actions and Actions.ActionArgs

* duplicates. In each action, the relevant account will be referred-to by its
* index in the list.
* @param actions An ordered list of all actions that will be taken in this operation. The
* actions will be processed in order.

This comment has been minimized.

Copy link
@AntonioJuliano

AntonioJuliano Apr 12, 2019

Member

[not this line] it'd be really helpful to have detailed comments on each action type and what all the fields for that struct mean

BrendanChou added some commits Apr 13, 2019

@BrendanChou BrendanChou force-pushed the bc_docstring branch from f86de26 to babd773 Apr 15, 2019

Show resolved Hide resolved contracts/protocol/Getters.sol Outdated
Show resolved Hide resolved contracts/protocol/lib/Account.sol Outdated
Show resolved Hide resolved contracts/protocol/lib/Actions.sol
@AntonioJuliano
Copy link
Member

left a comment

Excellent 💯

@BrendanChou BrendanChou merged commit 2d8454e into master Apr 15, 2019

6 checks passed

ci/circleci: build_contracts Your tests passed on CircleCI!
Details
ci/circleci: build_js Your tests passed on CircleCI!
Details
ci/circleci: checkout_and_install Your tests passed on CircleCI!
Details
ci/circleci: coverage Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details

@BrendanChou BrendanChou deleted the bc_docstring branch Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.