Skip to content

Conversation

@nksazonov
Copy link
Contributor

No description provided.

@nksazonov nksazonov changed the title feat(vault): add CooldownAuthorizer almost finished proposal feat(vault): add UnbondingPeriodAuthorizer Mar 21, 2025
Comment on lines +6 to +8
import {Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol";
import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
Copy link
Contributor

Choose a reason for hiding this comment

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

Imports are not sorted, but should be (according to our Solidity Style Guide, sections 8.B and 8.C)

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 point! Thanks

Comment on lines +159 to +163
function authorize(
address owner,
address token,
uint256 // amount - not used
) public view override returns (bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When implementing an interface function, the style guide recommends not using the override keyword.

Style Guide, section 2.A: "Do not use override keyword when implementing an interface function."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, thanks

Comment on lines +222 to +224
function completeUnbondingRequest(address token) external {
_completeUnbondingRequest(msg.sender, token);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the Operational Advice section (3.A.2: "Avoid using msg.sender for permissionless functions.") this function should take an explicit address account parameter to improve interoperability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is not permissionless as a user should be able to complete only their own unbonding requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I will modify the clarify the formulation of "permissionless functions" in Operational Advice.

@nksazonov nksazonov merged commit b24a350 into master Mar 25, 2025
2 checks passed
@nksazonov nksazonov deleted the feat/cooldown-authorizer branch March 25, 2025 12:06
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