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.

Sign up for GitHub

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

Jump to bottom

Add PCV Deposit to PCV Guardian #355

Merged
merged 5 commits into from
Dec 6, 2021
Merged

Add PCV Deposit to PCV Guardian #355

merged 5 commits into from
Dec 6, 2021

Conversation

Joeysantoro
Copy link
Contributor

No description provided.

kryptoklob
kryptoklob previously requested changes Dec 5, 2021
View reviewed changes
Copy link
Contributor

@kryptoklob kryptoklob left a comment

Choose a reason for hiding this comment

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

Good addition; some natspec fixes to do and please see my comment about try/catch for the deposit.

contracts/pcv/IPCVGuardian.sol Show resolved Hide resolved
contracts/pcv/IPCVGuardian.sol Show resolved Hide resolved
contracts/pcv/IPCVGuardian.sol Show resolved Hide resolved
contracts/pcv/PCVGuardian.sol Show resolved Hide resolved
contracts/pcv/PCVGuardian.sol Show resolved Hide resolved
contracts/pcv/PCVGuardian.sol Show resolved Hide resolved
contracts/pcv/PCVGuardian.sol Show resolved Hide resolved
contracts/pcv/PCVGuardian.sol Show resolved Hide resolved
contracts/pcv/PCVGuardian.sol Show resolved Hide resolved
kryptoklob
kryptoklob approved these changes Dec 6, 2021
View reviewed changes
Copy link
Contributor

@kryptoklob kryptoklob left a comment

Choose a reason for hiding this comment

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

Seems good but somewhat confused about the unnecessary require addition.

contracts/pcv/PCVGuardian.sol
@@ -49,6 +49,7 @@ contract PCVGuardian is IPCVGuardian, CoreRef {
/// @notice batch version of setSafeAddress
/// @param _safeAddresses the addresses to set as safe, as calldata
function setSafeAddresses(address[] calldata _safeAddresses) external override onlyGovernorOrAdmin {
require(_safeAddresses.length != 0, "empty");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why add this? Seems unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good practice to sanity check inputs

kryptoklob
kryptoklob previously requested changes Dec 6, 2021
View reviewed changes
Copy link
Contributor

@kryptoklob kryptoklob left a comment

Choose a reason for hiding this comment

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

Unit tests are failing, please fix before final approval.

eswak
eswak approved these changes Dec 6, 2021
View reviewed changes
Copy link
Contributor

@eswak eswak left a comment

Choose a reason for hiding this comment

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

ok, just need to fix the unit tests (add a parameter for the new depositAfter boolean arg, or add a 2nd signature in the contract that uses false by default)

@Joeysantoro
Copy link
Contributor Author

Unit tests are failing, please fix before final approval.

done

@Joeysantoro Joeysantoro merged commit e340056 into feat/v2/base Dec 6, 2021
@Joeysantoro Joeysantoro deleted the feat/add-deposit-guardian branch December 6, 2021 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eswak eswak eswak approved these changes

@kryptoklob kryptoklob kryptoklob left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Joeysantoro @eswak @kryptoklob