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

Fix forge docs #750

Merged
merged 13 commits into from
Apr 20, 2023
Merged

Fix forge docs #750

merged 13 commits into from
Apr 20, 2023

Conversation

grandizzy
Copy link
Contributor

Copy link
Contributor

@ith-harvey ith-harvey left a comment

Choose a reason for hiding this comment

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

As I was reviewing I found the function _encumberance to be confusing... If we only ever use this method to determine encumbered collateral I think the comment should indicate that is it's sole use case instead of what we have now ->

Calculates encumberance for a debt amount at a given price.

Names that lack descriptions in forge docs:

  • ajnaPool_ in auctionStatus
  • params in burn
  • tokenId_ in claimRewards
  • epochToClaim_ in claimRewards
  • tokenId_ in stake
  • tokenId_ in unstake
  • pool_ and indexes_ in updateBucketExchangeRatesAndClaim
  • tokenId_ and epochToCLaim_ in calculateRewards
  • tokenId_ and bucketId_ in getBucketStateStakeInfo
  • rewardsClaimedInEpoch_ in _calculateNewRewards

That is a start ^^ there are alot more. Worth manually reviewing versus me typing them all out : )

src/ERC20Pool.sol Show resolved Hide resolved
src/ERC20Pool.sol Show resolved Hide resolved
src/ERC20Pool.sol Show resolved Hide resolved
src/ERC20Pool.sol Show resolved Hide resolved
@grandizzy
Copy link
Contributor Author

grandizzy commented Apr 19, 2023

As I was reviewing I found the function _encumberance to be confusing... If we only ever use this method to determine encumbered collateral I think the comment should indicate that is it's sole use case instead of what we have now ->

Calculates encumberance for a debt amount at a given price.

Names that lack descriptions in forge docs:

  • ajnaPool_ in auctionStatus
  • params in burn
  • tokenId_ in claimRewards
  • epochToClaim_ in claimRewards
  • tokenId_ in stake
  • tokenId_ in unstake
  • pool_ and indexes_ in updateBucketExchangeRatesAndClaim
  • tokenId_ and epochToCLaim_ in calculateRewards
  • tokenId_ and bucketId_ in getBucketStateStakeInfo
  • rewardsClaimedInEpoch_ in _calculateNewRewards

That is a start ^^ there are alot more. Worth manually reviewing versus me typing them all out : )

Pls check with latest, they should be fixed. Takeaway is interface should have same params naming as impl in order to generate docs properly.

Copy link
Contributor

@ith-harvey ith-harvey left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@EdNoepel EdNoepel left a comment

Choose a reason for hiding this comment

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

Criticisms regarding scope and audit status:

  • We had some inconsistency with some interfaces having trailing underscores in parameter and return type names, and other interfaces with no trailing underscores. If we had not completed any audits, I would favor no trailing underscores in interfaces. Just read comment interface should have same params naming as impl in order to generate docs properly; understood.
  • Making LPs singular seems outside the purported scope of this change.

Other criticisms:

  • I don't see why the term NFT should be backtick-quoted everywhere.

These concerns seem to be forge doc limitations:

  • Not a fan of the === sprinkled everywhere, but we seemingly don't have much freedom to adjust formatting in forge doc output.
  • Emitted events are not linked to the event definition; reader must hunt to determine arguments.
  • Non-exposed methods should not be included in generated documentation.
  • Free functions do not appear within the context of their containing library.

There are some valuable documentation improvements within, so in all it's a net positive. I don't think there is value adding a Makefile target. Happy to approve after a few minor inline concerns are addressed.

src/libraries/internal/Deposits.sol Show resolved Hide resolved
src/libraries/internal/Deposits.sol Outdated Show resolved Hide resolved
src/libraries/internal/Loans.sol Show resolved Hide resolved
src/libraries/internal/Loans.sol Outdated Show resolved Hide resolved
@grandizzy
Copy link
Contributor Author

Other criticisms:

* I don't see why the term _NFT_ should be backtick-quoted everywhere.

Yeah, just wanted to make them inline with ERC20 and ERC721

These concerns seem to be forge doc limitations:

* Not a fan of the `===` sprinkled everywhere, but we seemingly don't have much freedom to adjust formatting in `forge doc` output.

was the best way I found to highlight different sections, can use different separator if there's a nicer one

@grandizzy grandizzy requested a review from EdNoepel April 20, 2023 07:30
Copy link
Contributor

@EdNoepel EdNoepel left a comment

Choose a reason for hiding this comment

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

Looks good; thanks for this tedious work.

Copy link
Collaborator

@MikeHathaway MikeHathaway left a comment

Choose a reason for hiding this comment

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

LGTM

@grandizzy grandizzy merged commit b97455f into develop Apr 20, 2023
@grandizzy grandizzy deleted the forge-doc branch April 20, 2023 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants