[DRAFT] Proposal for Keyring Network integration#571
[DRAFT] Proposal for Keyring Network integration#571fdemiramon wants to merge 3 commits intoVenusProtocol:developfrom
Conversation
fdemiramon
changed the title
| /// @param _keyringChecker The address of the keyring checker. | ||
| /// @param _keyringPolicyId The policy id of the keyring. | ||
| function setKeyringConfiguration(address _keyringChecker, uint32 _keyringPolicyId) public { | ||
| ensureAdmin(msg.sender); |
There was a problem hiding this comment.
| ensureAdmin(msg.sender); | |
| ensureAdmin(); |
| /// @notice Sets the keyring configuration. | ||
| /// @param _keyringChecker The address of the keyring checker. | ||
| /// @param _keyringPolicyId The policy id of the keyring. | ||
| function setKeyringConfiguration(address _keyringChecker, uint32 _keyringPolicyId) public { |
There was a problem hiding this comment.
We usually add the setter functions in the SetterFacet facet
| /// @notice Enables or disables the keyring guard. | ||
| /// @param _enabled Whether to enable or disable the keyring guard. | ||
| function enableKeyringGuard(bool _enabled) public { | ||
| ensureAdmin(msg.sender); |
There was a problem hiding this comment.
We could check this permission in the ACM contract:
| ensureAdmin(msg.sender); | |
| ensureAllowed("enableKeyringGuard(bool)"); |
| /// @param _keyringPolicyId The policy id of the keyring. | ||
| function setKeyringConfiguration(address _keyringChecker, uint32 _keyringPolicyId) public { | ||
| ensureAdmin(msg.sender); | ||
| if (address(_keyringChecker) == address(0) || _keyringPolicyId == 0) { |
There was a problem hiding this comment.
We use require instead of revert in these contracts. Moreover, we could reuse ensureNonzeroAddress:
| if (address(_keyringChecker) == address(0) || _keyringPolicyId == 0) { | |
| ensureNonzeroAddress(_keyringChecker); | |
| require(_keyringPolicyId != 0, "can't be zero"); |
| } | ||
| keyringChecker = IKeyringChecker(_keyringChecker); | ||
| keyringPolicyId = _keyringPolicyId; | ||
| emit KeyringConfigurationUpdated(address(_keyringChecker), _keyringPolicyId); |
There was a problem hiding this comment.
I think we don't need the cast to address, because _keyringChecker is already an address
| uint256 actualRepayAmount, // solhint-disable-line no-unused-vars | ||
| uint256 borrowerIndex // solhint-disable-line no-unused-vars | ||
| ) external { | ||
| ensureUserAllowed(payer); |
There was a problem hiding this comment.
I think this is not needed, because we are already checking the permission for repaying in the repayBorrowAllowed check (the "pre-hook")
| address borrower, | ||
| uint256 repayAmount // solhint-disable-line no-unused-vars | ||
| ) external returns (uint256) { | ||
| ensureUserAllowed(payer); |
There was a problem hiding this comment.
| ensureUserAllowed(payer); | |
| ensureUserAllowed(payer); | |
| ensureUserAllowed(borrower); |
There was a problem hiding this comment.
We could check both wallets, but theoretically borrower will be allowed, because we checked it in the borrow flow. So, maybe it's not needed
| uint256 actualRepayAmount, // solhint-disable-line no-unused-vars | ||
| uint256 seizeTokens // solhint-disable-line no-unused-vars | ||
| ) external { | ||
| ensureUserAllowed(liquidator); |
There was a problem hiding this comment.
I think this is not needed, because we are already checking the permission for liquidating in the liquidateBorrowAllowed check (the "pre-hook")
| address borrower, | ||
| uint256 seizeTokens // solhint-disable-line no-unused-vars | ||
| ) external { | ||
| ensureUserAllowed(liquidator); |
There was a problem hiding this comment.
I think this is not needed, because we are already checking the permission for seizing in the seizeAllowed check (the "pre-hook")
| */ | ||
| // solhint-disable-next-line no-unused-vars | ||
| function transferVerify(address vToken, address src, address dst, uint256 transferTokens) external { | ||
| ensureUserAllowed(dst); |
There was a problem hiding this comment.
I think this is not needed, because we are already checking the permission for transferring in the transferAllowed check (the "pre-hook")
fred-venus
force-pushed
the
develop
branch
6 times, most recently
from
f8786ac to
d1378e1
Compare
2 participants
Draft / Proposal. See doc https://docs.google.com/document/d/1VJPArKUcVyc5kqtiDIMWshQOqicKx4M-jxu721BWbL4/
Description
Resolves #
Checklist