Skip to content

Conversation

aalavandhan
Copy link
Member

TOB feedback:

1) I think it would be a good idea to add an event in `UFragments.setMonetaryPolicy` so an event is emitted when the `monetaryPolicy` address is updated, so as to be consistent with other event triggers in the contracts.

2) There's also some variable shadowing happening in a few functions. Consider prefixing the function parameter names with an underscore in these cases for clarity. The relevant functions are `UFragmentsPolicy.initialize`, `UFragments.initialize` and `UFragments.allowance`. In each case it's the `owner` parameter shadowing so I'd suggest changing it to `_owner`.

3) I noticed there's a PR ready to be merged in `market-oracle` to enable compiler optimizations (and a similar one was merged earlier this month in `uFragments`). In light of the compiler audit released last fall as well as some recent high-severity bugs related to optimization issues, we currently caution against compiling with optimizations enabled unless the risk of potential latent optimizer bugs is carefully weighed against the gas savings optimizations would provide.

@ahnaguib
Copy link
Contributor

ahnaguib commented May 17, 2019 via email

Copy link
Member

@brandoniles brandoniles left a comment

Choose a reason for hiding this comment

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

LGTM thanks

@aalavandhan aalavandhan merged commit 0cc0a5b into master May 24, 2019
@aalavandhan aalavandhan deleted the dev branch May 24, 2019 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants