Skip to content

[P1-N07] Contract naming updates#1227

Merged
chrismaree merged 17 commits intomasterfrom
naming-updates
Apr 17, 2020
Merged

[P1-N07] Contract naming updates#1227
chrismaree merged 17 commits intomasterfrom
naming-updates

Conversation

@chrismaree
Copy link
Copy Markdown
Member

@chrismaree chrismaree commented Apr 13, 2020 •
edited
Loading

This PR re-names a number of internal variables and functions to be more descriptive and consistent. Most changes are small and internal variables. A number of changes requested by OZ have been left out as these will have ripple effects throughout the code base and it was decided that the value add in make these changes is less than the cost of updating other parts of the infrastructure.

The following changes were made:

  • In the Withdrawable contract The owner parameter of the createWithdrawRole was be renamed to withdrawerAddress.
  • In the Store contract The fixedOracleFeePerSecond and weeklyDelayFee variables were renamed to fixedOracleFeePerSecondPerPfc and weeklyDelayFeePerPfc. This includes all other references to fees like events.
  • In the FixedPoint contract the divRaw variable was renamed to aScaled since it has nothing to do with division.

The following changes recommended by OZ were not implemented:

  • All changes to multiRole as this will have a large ripple effect throughout the code base.
  • Renaming EncryptedStore as this is removed in a later PR.
  • Governor change _uintToBytes to _uintToBytes32 as this is addressed in another PR.
  • Changing getPendingRequests() to getActiveRequests() as this has a large ripple effect for minimum benefit.

@chrismaree chrismaree changed the title Naming updates [P1-N07] Contract naming updates Apr 13, 2020
@chrismaree chrismaree added the audit-fix-phase1 Fixes for Phase 1 (DVM) of the OZ audit label Apr 13, 2020
@chrismaree chrismaree marked this pull request as ready for review April 13, 2020 09:29
nicholaspai
nicholaspai reviewed Apr 13, 2020
View reviewed changes
Comment thread core/contracts/common/implementation/MultiRole.sol Outdated
nicholaspai
nicholaspai reviewed Apr 13, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@nicholaspai nicholaspai 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, I'm going to add comments where contract changes will need to be adjusted for in non /test scripts

nicholaspai
nicholaspai reviewed Apr 13, 2020
View reviewed changes
Comment thread core/contracts/common/implementation/MultiRole.sol Outdated
nicholaspai
nicholaspai reviewed Apr 13, 2020
View reviewed changes
Comment thread core/contracts/oracle/implementation/Store.sol
nicholaspai
nicholaspai reviewed Apr 13, 2020
View reviewed changes
Comment thread core/contracts/oracle/implementation/Voting.sol
nicholaspai
nicholaspai reviewed Apr 13, 2020
View reviewed changes
Comment thread core/contracts/common/implementation/MultiRole.sol Outdated
nicholaspai
nicholaspai reviewed Apr 13, 2020
View reviewed changes
Comment thread core/contracts/common/implementation/MultiRole.sol Outdated
@nicholaspai
Copy link
Copy Markdown
Member

nicholaspai commented Apr 13, 2020

@chrismaree looks good, I noticed you didn't implement the following suggestions:

  • Changing addMember to addSharedMember
  • Changing removeMember to removeSharedMember
  • In Governor: change _uintToBytes() to _uintToBytes32()

Just wanted to flag in case you didn't see, or curious if you decided it wasn't worth it. The MultiRole changes have a ton of upstream calls.

@chrismaree
Copy link
Copy Markdown
Member Author

chrismaree commented Apr 14, 2020

@chrismaree looks good, I noticed you didn't implement the following suggestions:

  • Changing addMember to addSharedMember
  • Changing removeMember to removeSharedMember
  • In Governor: change _uintToBytes() to _uintToBytes32()

Just wanted to flag in case you didn't see, or curious if you decided it wasn't worth it. The MultiRole changes have a ton of upstream calls.

Great. So regarding those suggestions:

1 &2) Multiroll has two libraries in it: Exclusive and Shared. These are implemented in the abstract base contract MultiRole. I didn't change any implementation in the MultiRole contract but rather only in the libraries, as suggested in the OZ audit. This means that the upstream usage of contracts that inherit from MultiRole don't need to change. This means that I did implement the changes for going from addMember -> addSharedMember & removeMember -> removeSharedMember within the library but I did not change the abstract class as this has far reaching implications up stream. My understanding of the OZ audit comment on these points was they they only wanted changes at the library level, not within the abstract contract.

  1. @mrice32 refactored this bad boy in another PR. see

@mrice32
Copy link
Copy Markdown
Member

mrice32 commented Apr 14, 2020

My understanding of the OZ audit comment on these points was they they only wanted changes at the library level, not within the abstract contract.

My understanding was that they wanted the method names to be changed at the contract level to communicate that those methods only work for shared or exclusive roles, not both.

mrice32
mrice32 reviewed Apr 15, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

Looks great! A few high level comments.

Comment thread core/contracts/oracle/implementation/Store.sol Outdated
function setWeeklyDelayFee(FixedPoint.Unsigned memory newWeeklyDelayFee) public onlyRoleHolder(uint(Roles.Owner)) {
weeklyDelayFee = newWeeklyDelayFee;
emit NewWeeklyDelayFee(newWeeklyDelayFee);
function setWeeklyDelayFeePerPFC(FixedPoint.Unsigned memory newWeeklyDelayFeePerPFC)
Copy link
Copy Markdown
Member

@mrice32 mrice32 Apr 15, 2020

Choose a reason for hiding this comment

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

This is actually becoming per second, too in this PR. Do you want to change it to setWeeklyDelayFeePerSecondPerPfc? Or should I change the name in the aforementioned PR? Something else?

Comment thread core/contracts/oracle/implementation/MessageStore.sol Outdated
* Only one message per topic is stored at any given time.
*/
contract EncryptedStore {
contract MessageStore {
Copy link
Copy Markdown
Member

@mrice32 mrice32 Apr 15, 2020

Choose a reason for hiding this comment

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

I would remove all the changes wrt EncryptedStore since the entire contract is being removed in #1231. We can add a note in the response doc saying that we didn't make that change because the contract was removed.

Copy link
Copy Markdown
Member Author

@chrismaree chrismaree Apr 16, 2020

Choose a reason for hiding this comment

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

done

Comment thread core/contracts/common/implementation/MultiRole.sol Outdated
}

function removeMember(RoleMembership storage roleMembership, address memberToRemove) internal {
function removeSharedMember(RoleMembership storage roleMembership, address memberToRemove) internal {
Copy link
Copy Markdown
Member

@mrice32 mrice32 Apr 15, 2020

Choose a reason for hiding this comment

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

Should we get rid of all the multirole changes given that we don't want the interface to change? (I know these are internal changes, but I think they actually wanted us to change the external function names)

Copy link
Copy Markdown
Member Author

@chrismaree chrismaree Apr 16, 2020

Choose a reason for hiding this comment

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

ye, I've reverted all changes to multirole re our discussion. Thinks keeps things simple. no need to change more

@chrismaree
fixed merge and revert
c481452 
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
@chrismaree
merge
fccfd12 
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
@chrismaree
Revert bug_bounty.md
2a8745f
@chrismaree
updated named param
fdb185c 
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
@chrismaree
Copy link
Copy Markdown
Member Author

chrismaree commented Apr 16, 2020

@mrice32 and @nicholaspai please take another look. I've reverted all changes other than the ones we've discussed. I also updated the PR description to better reflect this.

@chrismaree
Merge branch 'naming-updates' of github.com:UMAprotocol/protocol into…
99df757 
… naming-updates

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
@chrismaree
fixed naming error
89d47f3 
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
@chrismaree
patched test
90efbd2 
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
@chrismaree
fixed case error
f7961c9 
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
mrice32
mrice32 reviewed Apr 16, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

Looks great -- one small nit

Comment thread core/contracts/oracle/implementation/Store.sol Outdated
FixedPoint.Unsigned public fixedOracleFeePerSecond; // Percentage of 1 E.g., .1 is 10% Oracle fee.
FixedPoint.Unsigned public weeklyDelayFee; // Percentage of 1 E.g., .1 is 10% weekly delay fee.
FixedPoint.Unsigned public fixedOracleFeePerSecondPerPfc; // Percentage of 1 E.g., .1 is 10% Oracle fee.
FixedPoint.Unsigned public weeklyDelayFeePerPfc; // Percentage of 1 E.g., .1 is 10% weekly delay fee.
Copy link
Copy Markdown
Member

@mrice32 mrice32 Apr 16, 2020

Choose a reason for hiding this comment

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

nit: shouldn't this variable also be changed to weeklyDelayFeePerSecondPerPfc?

Copy link
Copy Markdown
Member Author

@chrismaree chrismaree Apr 16, 2020

Choose a reason for hiding this comment

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

🤦 I've changed this so many times, sorry this got through, I have updated accordingly.

@chrismaree
fix nit
a4805f8 
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
@chrismaree
merged master
be6f526 
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
@chrismaree
small nit
78ebb59 
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
@chrismaree
fixed store.js
b679d5a 
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
ptare added 2 commits April 16, 2020 19:20
@ptare @chrismaree
[P1-N15] Remove EncryptedStore and emit an extra event instead (#1231)
953bdcc 
The encrypted vote information is only needed off chain, so an event is
a simpler alternative to storing/deleting messages on chain.

Signed-off-by: Prasad Tare prasad.tare@gmail.com
@ptare @chrismaree
Wording and copy fixes to the sponsor CLI (#1243)
ced8b6e 
Also add more details to the expired markets.

Issue #1156

Signed-off-by: Prasad Tare <prasad.tare@gmail.com>
@mrice32
Copy link
Copy Markdown
Member

mrice32 commented Apr 16, 2020

@chrismaree Looks like ci is magically fixed from before, but the test errors now are due to merge issues, I think.

@chrismaree
Patched changes to contracts
c5c0eb1 
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
@chrismaree
resolvd merge
2d28ede 
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
@chrismaree
Revert bug_bounty.md
c55543c
@chrismaree
Copy link
Copy Markdown
Member Author

chrismaree commented Apr 17, 2020

CI is passing and all looks good here fore me. @mrice32 or @ptare can you give a final look over before merging this?

mrice32
mrice32 approved these changes Apr 17, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

LGTM!

@chrismaree chrismaree merged commit a051f7c into master Apr 17, 2020
@chrismaree chrismaree deleted the naming-updates branch April 17, 2020 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicholaspai nicholaspai nicholaspai approved these changes

@mrice32 mrice32 mrice32 approved these changes

Assignees

No one assigned

Labels

audit-fix-phase1 Fixes for Phase 1 (DVM) of the OZ audit

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chrismaree @nicholaspai @mrice32 @ptare