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

Implement IPRBProxyStorage interface #84

Merged
merged 2 commits into from
Mar 8, 2023
Merged

Implement IPRBProxyStorage interface #84

merged 2 commits into from
Mar 8, 2023

Conversation

andreivladbrg
Copy link
Collaborator

The purpose of this PR is to remove duplicated code in PRBProxy contract.

Changes made:

  • Added IPRBProxyStorage interface
  • Inherit IPRBProxyStorage in IPRBProxy
  • Inherit PRBProxyStorage in PRBProxy
  • Moved public constant functions from PRBProxy to PRBProxyStorage
  • Removed internal and public storage from PRBProxy

@andreivladbrg
feat: add "IPRBProxyStorage" interface
a5400f2 
feat: inherit "PRBProxyStorage" in "PRBProxy"
feat: inherit "IPRBProxyStorage" in "IPRBProxyStorage"
refactor: move constant functions to "PRBProxyStorage"
docs: update natspec comments
@PaulRBerg
Copy link
Owner

I appreciate the effort, @andreivladbrg, but unfortunately, I cannot merge this proposal.

The abstract contract PRBProxyStorage should not have the getPermission and the getPluginForMethod functions. It should only contain storage properties, because target contract developers may not need those getters.

@andreivladbrg
Copy link
Collaborator Author

The abstract contract PRBProxyStorage should not have the getPermission and the getPluginForMethod functions. It should only contain storage properties, because target contract developers may not need those getters.

In that case the mappings can be set as public, with new version of solidity you can have named values, check this link.

@andreivladbrg
refactor: make mappings public
e16f82a 
refactor: remove getter functions
test: use the mappings instead of getters
chore: remove unused import
@PaulRBerg
Copy link
Owner

PaulRBerg commented Mar 8, 2023 •
edited
Loading

As we discussed on the phone earlier, using public mappings is a fantastic idea!

The deployment cost is lowered, while the UX remains the same thanks to Solidity 0.8.18 named mapping parameters!

Update: no deployment cost change, unfortunately. See v4.0.0-beta.3.

github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 8, 2023
View reviewed changes
src/PRBProxyStorage.sol
/// @dev Maps plugin methods to plugin implementation.
mapping(bytes4 method => IPRBProxyPlugin plugin) internal plugins;
/// @inheritdoc IPRBProxyStorage
mapping(address envoy => mapping(address target => bool permission)) public permissions;

Check failure

Code scanning / Slither

Uninitialized state variables

PRBProxyStorage.permissions (src/PRBProxyStorage.sol#21) is never initialized. It is used in: - PRBProxy.execute(address,bytes) (src/PRBProxy.sol#73-104)
src/PRBProxyStorage.sol
/// @dev Maps envoys to target contracts to function selectors to boolean flags.
mapping(address envoy => mapping(address target => bool permission)) internal permissions;
/// @inheritdoc IPRBProxyStorage
mapping(bytes4 method => IPRBProxyPlugin plugin) public plugins;

Check failure

Code scanning / Slither

Uninitialized state variables

PRBProxyStorage.plugins (src/PRBProxyStorage.sol#24) is never initialized. It is used in: - PRBProxy.fallback(bytes) (src/PRBProxy.sol#37-63)
src/PRBProxyStorage.sol
/// @notice The address of the owner account or contract.
address public owner;
/// @inheritdoc IPRBProxyStorage
address public override owner;

Check warning

Code scanning / Slither

State variables that could be declared constant

PRBProxyStorage.owner (src/PRBProxyStorage.sol#15) should be constant
src/PRBProxyStorage.sol
/// @notice The address of the owner account or contract.
address public owner;
/// @inheritdoc IPRBProxyStorage
address public override owner;

Check warning

Code scanning / Slither

State variables that could be declared immutable

PRBProxyStorage.owner (src/PRBProxyStorage.sol#15) should be immutable
src/PRBProxyStorage.sol
/// @dev This prevents the proxy from becoming unusable if EVM opcode gas costs change in the future.
uint256 public minGasReserve;
/// @inheritdoc IPRBProxyStorage
uint256 public override minGasReserve;

Check warning

Code scanning / Slither

State variables that could be declared immutable

PRBProxyStorage.minGasReserve (src/PRBProxyStorage.sol#18) should be immutable
PaulRBerg
PaulRBerg approved these changes Mar 8, 2023
View reviewed changes
Copy link
Owner

@PaulRBerg PaulRBerg 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 a lot, Andrei!

The Slither reports are not relevant. Will merge now.

@PaulRBerg PaulRBerg merged commit 0eca0f3 into PaulRBerg:main Mar 8, 2023
@andreivladbrg andreivladbrg deleted the andrei/storage-interface branch May 1, 2023 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PaulRBerg PaulRBerg PaulRBerg approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@andreivladbrg @PaulRBerg