Search code, repositories, users, issues, pull requests...

Copy link

Contributor

@richardliang richardliang commented Oct 14, 2021 •

edited

Loading

Code Review Processes

New Feature Review

Before submitting a pull request for new review, make sure the following is done:

Design doc is created and posted here: STIP 006 - Trade Rebates STIPS#12
Code cleanliness and completeness is addressed via guidelines

README Checks

[] README has proper context for the reviewer to understand what the code includes, any important design considerations, and areas to pay more attention to

Code Checks

[] Add explanatory comments. If there is complex code that requires specific context or understanding, note that in a comment
[] Remove unncessary comments. Any comments that do not add additional context, information, etc. should be removed
[] Add javadocs.
[] Scrub through the code for inconsistencies (e.g. removing extra spaces)
[] Ensure there are not any .onlys in spec files

Broader Considerations

[] Ensure variable, function and event naming is clear, consistent, and reflective for the scope of the code.
[] Consider if certain pieces of logic should be placed in a different library, module

richardliang added 4 commits

October 14, 2021 11:36


 Commit original modules for diff

97fb66a


 Add TradeModule V2

58bb326


 Add GIMV2

37a39ea


 Fix coveralls

7de5c91

@richardliang richardliang changed the title ~~(削除) [WIP] - TradeModuleV2 and GeneralIndexModuleV2 (削除ここまで)~~ (追記) TradeModuleV2 and GeneralIndexModuleV2 (追記ここまで)

Oct 15, 2021

@bweick

Copy link

Contributor

bweick commented Oct 21, 2021

Commenting the diff for TradeModule and GeneralIndexModule

bweick

bweick reviewed

contracts/protocol/modules/TradeModuleV2.sol

internal

returns (uint256 protocolFee, uint256 managerRebate)

{

uint256 totalFeePercentage = controller.getModuleFee(address(this), TRADE_MODULE_V2_TOTAL_FEE_INDEX);

Copy link

Contributor

@bweick bweick Oct 22, 2021

Choose a reason for hiding this comment

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

Why not use:

uint256 totalFees = getModuleFee(TRADE_MODULE_PROTOCOL_FEE_INDEX, _exchangedQuantity);
managerRebate = controller.getModuleFee(address(this), TRADE_MODULE_V2_MANAGER_REBATE_SPLIT_INDEX).preciseMul(totalFees);
protocolFee = totalFees.sub(managerRebate);

I think that way you can be more confident about not having some extra wei sent out due to rounding and you aren't recalculating totalFeePercentage.preciseMul(_exchangedQuantity)

bweick

bweick reviewed

contracts/protocol/modules/TradeModuleV2.sol

* @param _setToken Instance of the SetToken to update fee recipient

* @param _newRebateRecipient New rebate fee recipient address

function updateFeeRecipient(

Copy link

Contributor

@bweick bweick Oct 22, 2021

Choose a reason for hiding this comment

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

Maybe make this clearer and call it updateRebateRecipient

bweick

bweick reviewed

contracts/protocol/modules/GeneralIndexModuleV2.sol

* @param _setToken Instance of the SetToken to update fee recipient

* @param _newRebateRecipient New rebate fee recipient address

function updateFeeRecipient(

Copy link

Contributor

@bweick bweick Oct 22, 2021

Choose a reason for hiding this comment

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

Would call this updateRebateRecipient as well

bweick

bweick reviewed