improve(dec-20-audit): [N04] Naming issues hinder code understanding

[pemulis]

…and readability

Motivation

Improve naming according to audit recommendations, except in cases which would require upgrading contracts that are already live.

Summary

There are numerous naming updates here, concentrated on the new contracts that have yet to be released, and setting aside existing contracts that would need to be upgraded.

Details

We may want to go back and take a second look at the other contracts that we may want to more thoroughly refactor and upgrade later. I'm also seeing some failing tests and would like to better understand if this is a code issue (bytecode limit problems?) or some deficiency in my current testing set-up.

Issue(s)

N/A

[nicholaspai]

High level comments

• Since we're going to be upgrading the EMP related contracts as well do you think its worth making the changes to them now? Or should we just not touch the EMP at all.
• What about changing the Liquidatable states: {PreDispute, PendingDispute}
• What about other changes in the audit suggestions such as renaming dispute etc.

[nicholaspai]

High level comments

• Since we're going to be upgrading the EMP related contracts as well do you think its worth making the changes to them now? Or should we just not touch the EMP at all.
• What about changing the Liquidatable states: {PreDispute, PendingDispute}
• What about other changes in the audit suggestions such as renaming dispute etc.

Apologies @pemulis I'm going to take back my statements and just go through the audit suggestions for renaming one by one to aid you. The overall philosophy I'm going to recommend stays true to your original goal, which is to NOT introduce any breaking changes to the off-chain infrastructure such as the liquidator or monitor:

Changes we should make:

• PreDispute to NotDisputed.
• PendingDispute to Disputed.

These suggestions would change internal enums and should therefore be implemented for both the EMP and Perpetual.

• disputeBondPct to disputeBondPercentage.
• sponsorDisputeRewardPct to sponsorDisputeRewardPercentage.
• disputerDisputeRewardPct to disputerDisputeRewardPercentage.
• rewardRate and proposerBond in both events to rewardRatePerSecond and proposerBondPercentage respectively.

These suggestions introduce breaking changes to the interface but are not called by the bots, and therefore should be implemented for both the EMP and Perpetual.

• regularFees to paysRegularFees.

This is an internal modifier renaming and should be changed for both FeePayer and FundingRateApplier.

- _collateralAddress to _collateralTokenAddress.

This is a constructor argument renaming and should be changed for both EMP and Perpetual.

• proposeNewRate to proposeFundingRate.

This changes the interface for the Perpetual, which has not been integrated into any off-chain infra yet, so can be changed.

Changes we should not make:

• _collateralAddress to _collateralTokenAddress.

See full explanation here

• _getIdentifierWhitelist to _getIdentifierApprovedList.

I actually don't agree with this change because the contract being returned is named IdentifierWhitelist so it makes sense to leave this function name as is.

• dispute to disputeLiquidation.
• tokenCurrency to syntheticToken.
• Deposit to Deposited.
• Withdrawal to Withdrawn.
• RequestWithdrawal to WithdrawalRequested.
• RequestWithdrawalExecuted to WithdrawalExecuted.
• RequestWithdrawalCanceled to WithdrawalCanceled.
• NewSponsor to SponsorAdded.
• Redeem to Redeemed.
• EmergencyShutdown to EmergencyShutdownEnabled.
• SettleEmergencyShutdown to EmergencyShutdownSettled.

These changes would introduce breaking changes to the interface functions and events that would break off-chain infra in affiliates and/or bot code. So I don't think these should be implemented.

@daywiss @chrismaree can you verify that these changes are ok with the affiliates package?
@mrice32 can you verify the bot implications?

[daywiss]

thanks @pemulis and @nicholaspai for the update and analysis. I agree we really should try to just go with non breaking changes here.

That said, any changes to emp events will break affiliates as it is. I'm leaning towards not changing the EMP because its high risk to break infrastructure in general. So far affiliates has only had to run with a single version of the EMP abi, but having to apply rewards across different ABI versions will be tricky ( but not impossible). I'm ok with the inconsistency between EMP and perp for now so id rather reduce code churn.

[mrice32]

@nicholaspai totally agree with your conclusions.

[nicholaspai]

Drive by: I thought we had decided to not change _collateralAddress ==> _collateralTokenAddress because we are leaving _tokenAddress and tokenCurrency as is?

I updated this comment to reflect this decision just now, apologies if you were going by that comment before!

[pemulis]

Drive by: I thought we had decided to not change _collateralAddress ==> _collateralTokenAddress because we are leaving _tokenAddress and tokenCurrency as is?

I updated this comment to reflect this decision just now, apologies if you were going by that comment before!

Yep! I was going by that comment but can revert that change.

[coveralls]

Coverage remained the same at 93.284% when pulling ebfd6ad on pemulis/dec-2020-audit-N04 into 26ef98f on master.