Skip to content

Commit

Permalink
ref: Address All Stable and Nightly Clippy Warnings (#437)
Browse files Browse the repository at this point in the history
<!--
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 #436, #385

#### 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.
-->

- [x] Tests
- [x] Documentation
- [x] Changelog
  • Loading branch information
0xNeshi authored Dec 9, 2024
2 parents 60bb984 + 69b6219 commit 6c4c5ec
Show file tree
Hide file tree
Showing 35 changed files with 219 additions and 194 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- Implement `MethodError` for `safe_erc20::Error`. #402
- Use `function_selector!` to calculate transfer type selector in `Erc1155`. #417
- Update internal functions of `Erc721` and `Erc721Consecutive` accept reference to `Bytes`. #437

### Fixed

Expand Down
1 change: 1 addition & 0 deletions clippy.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
doc-valid-idents = ["OpenZeppelin", ".."]
39 changes: 20 additions & 19 deletions contracts/src/access/control.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
//! Contract module that allows children to implement role-based access control
//! mechanisms. This is a lightweight version that doesn't allow enumerating
//! role members except through off-chain means by accessing the contract event
//! logs.
//! mechanisms.
//!
//! This is a lightweight version that doesn't allow enumerating role members
//! except through off-chain means by accessing the contract event logs.
//!
//! Roles are referred to by their `bytes32` identifier. These should be exposed
//! in the external API and be unique. The best way to achieve this is by using
Expand Down Expand Up @@ -429,18 +430,18 @@ mod tests {
contract.grant_role(ROLE.into(), ALICE).unwrap();
contract.grant_role(ROLE.into(), ALICE).unwrap();
let has_role = contract.has_role(ROLE.into(), ALICE);
assert_eq!(has_role, true);
assert!(has_role);
}

#[motsu::test]
fn not_granted_roles_can_be_revoked(contract: AccessControl) {
_grant_role_to_msg_sender(contract, AccessControl::DEFAULT_ADMIN_ROLE);

let has_role = contract.has_role(ROLE.into(), ALICE);
assert_eq!(has_role, false);
assert!(!has_role);
contract.revoke_role(ROLE.into(), ALICE).unwrap();
let has_role = contract.has_role(ROLE.into(), ALICE);
assert_eq!(has_role, false);
assert!(!has_role);
}

#[motsu::test]
Expand All @@ -449,18 +450,18 @@ mod tests {
contract._roles.setter(ROLE.into()).has_role.insert(ALICE, true);

let has_role = contract.has_role(ROLE.into(), ALICE);
assert_eq!(has_role, true);
assert!(has_role);
contract.revoke_role(ROLE.into(), ALICE).unwrap();
let has_role = contract.has_role(ROLE.into(), ALICE);
assert_eq!(has_role, false);
assert!(!has_role);
}

#[motsu::test]
fn non_admin_cannot_revoke_role(contract: AccessControl) {
contract._roles.setter(ROLE.into()).has_role.insert(ALICE, true);

let has_role = contract.has_role(ROLE.into(), ALICE);
assert_eq!(has_role, true);
assert!(has_role);
let err = contract.revoke_role(ROLE.into(), ALICE).unwrap_err();
assert!(matches!(err, Error::UnauthorizedAccount(_)));
}
Expand All @@ -472,18 +473,18 @@ mod tests {
contract.revoke_role(ROLE.into(), ALICE).unwrap();
contract.revoke_role(ROLE.into(), ALICE).unwrap();
let has_role = contract.has_role(ROLE.into(), ALICE);
assert_eq!(has_role, false);
assert!(!has_role);
}

#[motsu::test]
fn bearer_can_renounce_role(contract: AccessControl) {
_grant_role_to_msg_sender(contract, ROLE);

let has_role = contract.has_role(ROLE.into(), msg::sender());
assert_eq!(has_role, true);
assert!(has_role);
contract.renounce_role(ROLE.into(), msg::sender()).unwrap();
let has_role = contract.has_role(ROLE.into(), msg::sender());
assert_eq!(has_role, false);
assert!(!has_role);
}

#[motsu::test]
Expand All @@ -501,7 +502,7 @@ mod tests {
contract.renounce_role(ROLE.into(), sender).unwrap();
contract.renounce_role(ROLE.into(), sender).unwrap();
let has_role = contract.has_role(ROLE.into(), ALICE);
assert_eq!(has_role, false);
assert!(!has_role);
}

#[motsu::test]
Expand All @@ -520,7 +521,7 @@ mod tests {

contract.grant_role(ROLE.into(), ALICE).unwrap();
let has_role = contract.has_role(ROLE.into(), ALICE);
assert_eq!(has_role, true);
assert!(has_role);
}

#[motsu::test]
Expand All @@ -531,7 +532,7 @@ mod tests {
contract._roles.setter(ROLE.into()).has_role.insert(ALICE, true);
contract.revoke_role(ROLE.into(), ALICE).unwrap();
let has_role = contract.has_role(ROLE.into(), ALICE);
assert_eq!(has_role, false);
assert!(!has_role);
}

#[motsu::test]
Expand Down Expand Up @@ -570,26 +571,26 @@ mod tests {
#[motsu::test]
fn internal_grant_role_true_if_no_role(contract: AccessControl) {
let role_granted = contract._grant_role(ROLE.into(), ALICE);
assert_eq!(role_granted, true);
assert!(role_granted);
}

#[motsu::test]
fn internal_grant_role_false_if_role(contract: AccessControl) {
contract._roles.setter(ROLE.into()).has_role.insert(ALICE, true);
let role_granted = contract._grant_role(ROLE.into(), ALICE);
assert_eq!(role_granted, false);
assert!(!role_granted);
}

#[motsu::test]
fn internal_revoke_role_true_if_role(contract: AccessControl) {
contract._roles.setter(ROLE.into()).has_role.insert(ALICE, true);
let role_revoked = contract._revoke_role(ROLE.into(), ALICE);
assert_eq!(role_revoked, true);
assert!(role_revoked);
}

#[motsu::test]
fn internal_revoke_role_false_if_no_role(contract: AccessControl) {
let role_revoked = contract._revoke_role(ROLE.into(), ALICE);
assert_eq!(role_revoked, false);
assert!(!role_revoked);
}
}
3 changes: 3 additions & 0 deletions contracts/src/finance/vesting_wallet.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
//! A vesting wallet handles the vesting of Ether and ERC-20 tokens for a given
//! beneficiary.
//!
//! A vesting wallet is an ownable contract that can receive native currency and
//! [`crate::token::erc20::Erc20`] tokens, and release these assets to the
//! wallet owner, also referred to as "beneficiary", according to a vesting
Expand Down
6 changes: 3 additions & 3 deletions contracts/src/token/erc1155/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1203,7 +1203,7 @@ mod tests {
}

pub(crate) fn random_values(size: usize) -> Vec<U256> {
(1..size + 1).map(U256::from).collect()
(1..=size).map(U256::from).collect()
}

fn init(
Expand Down Expand Up @@ -1295,12 +1295,12 @@ mod tests {
contract
.set_approval_for_all(BOB, true)
.expect("should approve Bob for operations on all Alice's tokens");
assert_eq!(contract.is_approved_for_all(alice, BOB), true);
assert!(contract.is_approved_for_all(alice, BOB));

contract.set_approval_for_all(BOB, false).expect(
"should disapprove Bob for operations on all Alice's tokens",
);
assert_eq!(contract.is_approved_for_all(alice, BOB), false);
assert!(!contract.is_approved_for_all(alice, BOB));
}

#[motsu::test]
Expand Down
5 changes: 4 additions & 1 deletion contracts/src/token/erc20/extensions/permit.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
//! Permit Contract.
//!
//! Extension of the ERC-20 standard allowing approvals to be made
//! via signatures, as defined in EIP-2612.
//! via signatures, as defined in the [ERC].
//!
//! Adds the `permit` method, which can be used to change an account’s
//! ERC20 allowance (see [`crate::token::erc20::IErc20::allowance`])
//! by presenting a message signed by the account.
//! By not relying on [`crate::token::erc20::IErc20::approve`],
//! the token holder account doesn’t need to send a transaction,
//! and thus is not required to hold Ether at all.
//!
//! [ERC]: https://eips.ethereum.org/EIPS/eip-2612
use alloy_primitives::{b256, keccak256, Address, B256, U256};
use alloy_sol_types::{sol, SolType};
use stylus_sdk::{
Expand Down
14 changes: 7 additions & 7 deletions contracts/src/token/erc20/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -366,11 +366,11 @@ impl Erc20 {
/// # Errors
///
/// * If the `from` address is `Address::ZERO`, then the error
/// [`Error::InvalidSender`] is returned.
/// [`Error::InvalidSender`] is returned.
/// * If the `to` address is `Address::ZERO`, then the error
/// [`Error::InvalidReceiver`] is returned.
/// If the `from` address doesn't have enough tokens, then the error
/// [`Error::InsufficientBalance`] is returned.
/// [`Error::InvalidReceiver`] is returned.
/// * If the `from` address doesn't have enough tokens, then the error
/// [`Error::InsufficientBalance`] is returned.
///
/// # Events
///
Expand Down Expand Up @@ -514,9 +514,9 @@ impl Erc20 {
/// # Errors
///
/// * If the `from` address is `Address::ZERO`, then the error
/// [`Error::InvalidSender`] is returned.
/// If the `from` address doesn't have enough tokens, then the error
/// [`Error::InsufficientBalance`] is returned.
/// [`Error::InvalidSender`] is returned.
/// * If the `from` address doesn't have enough tokens, then the error
/// [`Error::InsufficientBalance`] is returned.
///
/// # Events
///
Expand Down
27 changes: 10 additions & 17 deletions contracts/src/token/erc20/utils/safe_erc20.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//! Wrappers around ERC-20 operations that throw on failure (when the token
//! contract returns false). Tokens that return no value (and instead revert or
//! contract returns false).
//!
//! Tokens that return no value (and instead revert or
//! throw on failure) are also supported, non-reverting calls are assumed to be
//! successful.
//!
Expand Down Expand Up @@ -389,7 +391,7 @@ impl SafeErc20 {
///
/// * `data` - Slice of bytes.
fn encodes_true(data: &[u8]) -> bool {
data.split_last().map_or(false, |(last, rest)| {
data.split_last().is_some_and(|(last, rest)| {
*last == 1 && rest.iter().all(|&byte| byte == 0)
})
}
Expand All @@ -400,40 +402,31 @@ mod tests {
use super::SafeErc20;
#[test]
fn encodes_true_empty_slice() {
assert_eq!(false, SafeErc20::encodes_true(&vec![]));
assert!(!SafeErc20::encodes_true(&[]));
}

#[test]
fn encodes_false_single_byte() {
assert_eq!(false, SafeErc20::encodes_true(&vec![0]));
assert!(!SafeErc20::encodes_true(&[0]));
}

#[test]
fn encodes_true_single_byte() {
assert_eq!(true, SafeErc20::encodes_true(&vec![1]));
assert!(SafeErc20::encodes_true(&[1]));
}

#[test]
fn encodes_false_many_bytes() {
assert_eq!(
false,
SafeErc20::encodes_true(&vec![0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
);
assert!(!SafeErc20::encodes_true(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]));
}

#[test]
fn encodes_true_many_bytes() {
assert_eq!(
true,
SafeErc20::encodes_true(&vec![0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1])
);
assert!(SafeErc20::encodes_true(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]));
}

#[test]
fn encodes_true_wrong_bytes() {
assert_eq!(
false,
SafeErc20::encodes_true(&vec![0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 1])
);
assert!(!SafeErc20::encodes_true(&[0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 1]));
}
}
Loading

0 comments on commit 6c4c5ec

Please sign in to comment.