ref: privatize and rename state fields (#500)

<!--
Thank you for your interest in contributing to OpenZeppelin!

Consider opening an issue for discussion prior to submitting a PR. New
features will be merged faster if they were first discussed and designed
with the team.

Describe the changes introduced in this pull request. Include any
context necessary for understanding the PR's purpose.
-->

<!-- Fill in with issue number -->
Resolves #447, resolves #386 

#### PR Checklist

<!--
Before merging the pull request all of the following must be completed.
Feel free to submit a PR or Draft PR even if some items are pending.
Some of the items may not apply.
-->

- [ ] Tests
- [x] Documentation
- [x] Changelog

---------

Co-authored-by: Alisander Qoshqosh <[email protected]>
Co-authored-by: Daniel Bigos <[email protected]>

CHANGELOG.md

@@ -41,6 +41,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed (Breaking)
 
+- All contract state fields are no longer public. #500
+- `Erc721Consecutive::_mint_consecutive` turned into an internal function. #500
 - Bump cargo-stylus to v0.5.8. #493
 - Constants `TYPE_HASH`, `FIELDS`, `SALT` and `TYPED_DATA_PREFIX`, and type `DomainSeparatorTuple` are no longer exported from `utils::cryptography::eip712`. #478
 - Bump Stylus SDK to v0.7.0. #433

contracts/src/access/control.rs

@@ -122,8 +122,7 @@ pub struct RoleData {
 #[storage]
 pub struct AccessControl {
     /// Role identifier -> Role information.
-    #[allow(clippy::used_underscore_binding)]
-    pub _roles: StorageMap<FixedBytes<32>, RoleData>,
+    pub(crate) roles: StorageMap<FixedBytes<32>, RoleData>,
 }
 /// Interface for an [`AccessControl`] contract.
 #[interface_id]
@@ -247,7 +246,7 @@ impl IAccessControl for AccessControl {
 
     #[must_use]
     fn has_role(&self, role: B256, account: Address) -> bool {
-        self._roles.getter(role).has_role.get(account)
+        self.roles.getter(role).has_role.get(account)
     }
 
     fn only_role(&self, role: B256) -> Result<(), Self::Error> {
@@ -256,7 +255,7 @@ impl IAccessControl for AccessControl {
 
     #[must_use]
     fn get_role_admin(&self, role: B256) -> B256 {
-        *self._roles.getter(role).admin_role
+        *self.roles.getter(role).admin_role
     }
 
     fn grant_role(
@@ -314,7 +313,7 @@ impl AccessControl {
     /// * [`RoleAdminChanged`].
     pub fn _set_role_admin(&mut self, role: B256, new_admin_role: B256) {
         let previous_admin_role = self.get_role_admin(role);
-        self._roles.setter(role).admin_role.set(new_admin_role);
+        self.roles.setter(role).admin_role.set(new_admin_role);
         evm::log(RoleAdminChanged {
             role,
             previous_admin_role,
@@ -366,7 +365,7 @@ impl AccessControl {
         if self.has_role(role, account) {
             false
         } else {
-            self._roles.setter(role).has_role.insert(account, true);
+            self.roles.setter(role).has_role.insert(account, true);
             evm::log(RoleGranted { role, account, sender: msg::sender() });
             true
         }
@@ -388,7 +387,7 @@ impl AccessControl {
     /// * [`RoleRevoked`].
     pub fn _revoke_role(&mut self, role: B256, account: Address) -> bool {
         if self.has_role(role, account) {
-            self._roles.setter(role).has_role.insert(account, false);
+            self.roles.setter(role).has_role.insert(account, false);
             evm::log(RoleRevoked { role, account, sender: msg::sender() });
             true
         } else {

contracts/src/access/ownable.rs

@@ -69,8 +69,7 @@ impl MethodError for Error {
 #[storage]
 pub struct Ownable {
     /// The current owner of this contract.
-    #[allow(clippy::used_underscore_binding)]
-    pub _owner: StorageAddress,
+    pub(crate) owner: StorageAddress,
 }
 
 /// Interface for an [`Ownable`] contract.
@@ -132,7 +131,7 @@ impl IOwnable for Ownable {
     type Error = Error;
 
     fn owner(&self) -> Address {
-        self._owner.get()
+        self.owner.get()
     }
 
     fn transfer_ownership(
@@ -193,8 +192,8 @@ impl Ownable {
     ///
     /// * [`OwnershipTransferred`].
     pub fn _transfer_ownership(&mut self, new_owner: Address) {
-        let previous_owner = self._owner.get();
-        self._owner.set(new_owner);
+        let previous_owner = self.owner.get();
+        self.owner.set(new_owner);
         evm::log(OwnershipTransferred { previous_owner, new_owner });
     }
 }
@@ -211,7 +210,7 @@ mod tests {
 
     #[motsu::test]
     fn reads_owner(contract: Contract<Ownable>, alice: Address) {
-        contract.init(alice, |contract| contract._owner.set(alice));
+        contract.init(alice, |contract| contract.owner.set(alice));
         let owner = contract.sender(alice).owner();
         assert_eq!(owner, alice);
     }
@@ -222,7 +221,7 @@ mod tests {
         alice: Address,
         bob: Address,
     ) {
-        contract.init(alice, |contract| contract._owner.set(alice));
+        contract.init(alice, |contract| contract.owner.set(alice));
 
         contract
             .sender(alice)
@@ -238,7 +237,7 @@ mod tests {
         alice: Address,
         bob: Address,
     ) {
-        contract.init(alice, |contract| contract._owner.set(bob));
+        contract.init(alice, |contract| contract.owner.set(bob));
 
         let err = contract.sender(alice).transfer_ownership(bob).unwrap_err();
         assert!(matches!(err, Error::UnauthorizedAccount(_)));
@@ -249,7 +248,7 @@ mod tests {
         contract: Contract<Ownable>,
         alice: Address,
     ) {
-        contract.init(alice, |contract| contract._owner.set(alice));
+        contract.init(alice, |contract| contract.owner.set(alice));
 
         let err = contract
             .sender(alice)
@@ -263,7 +262,7 @@ mod tests {
         contract: Contract<Ownable>,
         alice: Address,
     ) {
-        contract.init(alice, |contract| contract._owner.set(alice));
+        contract.init(alice, |contract| contract.owner.set(alice));
 
         contract
             .sender(alice)
@@ -279,7 +278,7 @@ mod tests {
         alice: Address,
         bob: Address,
     ) {
-        contract.init(alice, |contract| contract._owner.set(bob));
+        contract.init(alice, |contract| contract.owner.set(bob));
 
         let err = contract.sender(alice).renounce_ownership().unwrap_err();
         assert!(matches!(err, Error::UnauthorizedAccount(_)));
@@ -291,7 +290,7 @@ mod tests {
         alice: Address,
         bob: Address,
     ) {
-        contract.init(alice, |contract| contract._owner.set(bob));
+        contract.init(alice, |contract| contract.owner.set(bob));
 
         contract.sender(alice)._transfer_ownership(bob);
         let owner = contract.sender(alice).owner();

contracts/src/access/ownable_two_step.rs

@@ -61,11 +61,11 @@ pub enum Error {
 #[storage]
 pub struct Ownable2Step {
     /// [`Ownable`] contract.
-    #[allow(clippy::used_underscore_binding)]
-    pub _ownable: Ownable,
+    // We leave the parent [`Ownable`] contract instance public, so that
+    // inheritting contract have access to its internal functions.
+    pub ownable: Ownable,
     /// Pending owner of the contract.
-    #[allow(clippy::used_underscore_binding)]
-    pub _pending_owner: StorageAddress,
+    pub(crate) pending_owner: StorageAddress,
 }
 
 /// Interface for an [`Ownable2Step`] contract.
@@ -157,19 +157,19 @@ impl IOwnable2Step for Ownable2Step {
     type Error = Error;
 
     fn owner(&self) -> Address {
-        self._ownable.owner()
+        self.ownable.owner()
     }
 
     fn pending_owner(&self) -> Address {
-        self._pending_owner.get()
+        self.pending_owner.get()
     }
 
     fn transfer_ownership(
         &mut self,
         new_owner: Address,
     ) -> Result<(), Self::Error> {
-        self._ownable.only_owner()?;
-        self._pending_owner.set(new_owner);
+        self.ownable.only_owner()?;
+        self.pending_owner.set(new_owner);
 
         let current_owner = self.owner();
         evm::log(OwnershipTransferStarted {
@@ -193,7 +193,7 @@ impl IOwnable2Step for Ownable2Step {
     }
 
     fn renounce_ownership(&mut self) -> Result<(), Error> {
-        self._ownable.only_owner()?;
+        self.ownable.only_owner()?;
         self._transfer_ownership(Address::ZERO);
         Ok(())
     }
@@ -216,8 +216,8 @@ impl Ownable2Step {
     ///
     /// * [`crate::access::ownable::OwnershipTransferred`].
     fn _transfer_ownership(&mut self, new_owner: Address) {
-        self._pending_owner.set(Address::ZERO);
-        self._ownable._transfer_ownership(new_owner);
+        self.pending_owner.set(Address::ZERO);
+        self.ownable._transfer_ownership(new_owner);
     }
 }
 
@@ -234,7 +234,7 @@ mod tests {
     #[motsu::test]
     fn reads_owner(contract: Contract<Ownable2Step>, alice: Address) {
         contract.init(alice, |contract| {
-            contract._ownable._owner.set(alice);
+            contract.ownable.owner.set(alice);
         });
         let owner = contract.sender(alice).owner();
         assert_eq!(owner, alice);
@@ -247,7 +247,7 @@ mod tests {
         bob: Address,
     ) {
         contract.init(alice, |contract| {
-            contract._pending_owner.set(bob);
+            contract.pending_owner.set(bob);
         });
 
         let pending_owner = contract.sender(alice).pending_owner();
@@ -261,7 +261,7 @@ mod tests {
         bob: Address,
     ) {
         contract.init(alice, |contract| {
-            contract._ownable._owner.set(alice);
+            contract.ownable.owner.set(alice);
         });
 
         contract
@@ -280,7 +280,7 @@ mod tests {
         dave: Address,
     ) {
         contract.init(alice, |contract| {
-            contract._ownable._owner.set(bob);
+            contract.ownable.owner.set(bob);
         });
 
         let err = contract.sender(alice).transfer_ownership(dave).unwrap_err();
@@ -297,8 +297,8 @@ mod tests {
         bob: Address,
     ) {
         contract.init(alice, |contract| {
-            contract._ownable._owner.set(bob);
-            contract._pending_owner.set(alice);
+            contract.ownable.owner.set(bob);
+            contract.pending_owner.set(alice);
         });
 
         contract
@@ -317,8 +317,8 @@ mod tests {
         dave: Address,
     ) {
         contract.init(alice, |contract| {
-            contract._ownable._owner.set(bob);
-            contract._pending_owner.set(dave);
+            contract.ownable.owner.set(bob);
+            contract.pending_owner.set(dave);
         });
 
         let err = contract.sender(alice).accept_ownership().unwrap_err();
@@ -335,7 +335,7 @@ mod tests {
         bob: Address,
     ) {
         contract.init(alice, |contract| {
-            contract._ownable._owner.set(alice);
+            contract.ownable.owner.set(alice);
         });
 
         contract
@@ -356,7 +356,7 @@ mod tests {
     #[motsu::test]
     fn renounces_ownership(contract: Contract<Ownable2Step>, alice: Address) {
         contract.init(alice, |contract| {
-            contract._ownable._owner.set(alice);
+            contract.ownable.owner.set(alice);
         });
 
         contract
@@ -373,7 +373,7 @@ mod tests {
         bob: Address,
     ) {
         contract.init(alice, |contract| {
-            contract._ownable._owner.set(bob);
+            contract.ownable.owner.set(bob);
         });
 
         let err = contract.sender(alice).renounce_ownership().unwrap_err();
@@ -390,8 +390,8 @@ mod tests {
         bob: Address,
     ) {
         contract.init(alice, |contract| {
-            contract._ownable._owner.set(alice);
-            contract._pending_owner.set(bob);
+            contract.ownable.owner.set(alice);
+            contract.pending_owner.set(bob);
         });
 
         contract
@@ -409,8 +409,8 @@ mod tests {
         bob: Address,
     ) {
         contract.init(alice, |contract| {
-            contract._ownable._owner.set(alice);
-            contract._pending_owner.set(bob);
+            contract.ownable.owner.set(alice);
+            contract.pending_owner.set(bob);
         });
 
         contract
@@ -429,7 +429,7 @@ mod tests {
         dave: Address,
     ) {
         contract.init(alice, |contract| {
-            contract._ownable._owner.set(alice);
+            contract.ownable.owner.set(alice);
         });
 
         contract