Skip to content
This repository has been archived by the owner on Jan 13, 2023. It is now read-only.

Add docstrings and additional comments #234

Merged
merged 6 commits into from
Apr 15, 2019
Merged

Add docstrings and additional comments #234

merged 6 commits into from
Apr 15, 2019

Conversation

BrendanChou
Copy link

Fixes #199

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

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

@AntonioJuliano AntonioJuliano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent 💯

@BrendanChou BrendanChou merged commit 2d8454e into master Apr 15, 2019
@BrendanChou BrendanChou deleted the bc_docstring branch April 15, 2019 22:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add docstrings
2 participants