-
Notifications
You must be signed in to change notification settings - Fork 60
feat: update SafeErc20 to newer Solidity version #652
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.
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
base: main
Are you sure you want to change the base?
feat: update SafeErc20 to newer Solidity version #652
Conversation
…zero-reset strategy
…in ISafeErc20 trait
…xed method for clarity and consistency in ISafeErc20 trait
…contract implementation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…mon0x1800/OpenZeppelin-rust-contracts-stylus into feature/620-update-safe-erc20
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…g with calldata for improved error handling and clarity
…mon0x1800/OpenZeppelin-rust-contracts-stylus into feature/620-update-safe-erc20
…proved clarity and consistency
✅ Deploy Preview for contracts-stylus canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing, left some comments
/// * `to` - Account to transfer tokens to. | ||
/// * `value` - Number of tokens to transfer. | ||
/// | ||
/// # Returns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove # Returns
sections, we generally don't include this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refrain from resolving if the change request was not applied
examples/safe-erc20/src/lib.rs
Outdated
#[entrypoint] | ||
#[storage] | ||
struct SafeErc20Example { | ||
#[borrow] | ||
#[derive(Clone)] | ||
pub struct SafeErc20Example { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After fixing merge conflicts, make sure to use the new code (you'll see it after merging)
@@ -23,3 +23,57 @@ sol!( | |||
event Approval(address indexed owner, address indexed spender, uint256 value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expand the sol!
definition with the new functions
@@ -51,4 +51,4 @@ extern crate alloc; | |||
pub mod access; | |||
pub mod finance; | |||
pub mod token; | |||
pub mod utils; | |||
pub mod utils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format all code with cargo +nightly fmt --all
Hey @simon0x1800 , just checking in, have you had time to review the change requests? |
@0xNeshi sorry, I'll was a bit occupied with another project. Thanks for reviewing the pr. I'll apply the changes asap. |
@0xNeshi can you check now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks better.
For future reference, please address all of the change requests from previous code reviews before requesting a new review.
Thanks!
#[public] | ||
#[implements(IErc20<Error = Error>)] | ||
impl Erc20 {} | ||
|
||
#[public] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert this
#[derive(Debug)] | ||
#[allow(missing_docs)] | ||
error SafeErc20FailedOperation(address token); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why remove this?
#[public] | ||
#[implements(ISafeErc20<Error = Error>)] | ||
impl SafeErc20 {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why remove this?
/// * `to` - Account to transfer tokens to. | ||
/// * `value` - Number of tokens to transfer. | ||
/// | ||
/// # Returns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refrain from resolving if the change request was not applied
#[entrypoint] | ||
#[storage] | ||
struct SafeErc20Example { | ||
use alloy_primitives::{Address, U256}; | ||
use openzeppelin_stylus::token::erc20::utils::safe_erc20::SafeErc20; | ||
use stylus_sdk::prelude::*; | ||
|
||
#[derive(Clone)] | ||
pub struct SafeErc20Example { | ||
safe_erc20: SafeErc20, | ||
} | ||
|
||
#[public] | ||
#[implements(ISafeErc20<Error = safe_erc20::Error>)] | ||
#[inherit(SafeErc20)] | ||
impl SafeErc20Example {} | ||
|
||
#[public] | ||
impl ISafeErc20 for SafeErc20Example { | ||
type Error = safe_erc20::Error; | ||
#[external] | ||
impl SafeErc20Example { | ||
pub fn transfer_and_call( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this and use the new inheritance model
&mut self, | ||
token: Address, | ||
to: Address, | ||
value: U256, | ||
) -> Result<(), Self::Error> { | ||
self.safe_erc20.safe_transfer(token, to, value) | ||
data: Vec<u8>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data
should be stylus_sdk::abi::Bytes
|
||
#[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
pub struct SafeErc20; | ||
|
||
impl SafeErc20 { | ||
pub fn new(address: Address, wallet: &Wallet) -> Self { | ||
Self | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary?
This PR updates the SafeERC20 implementation to match the latest Solidity version, adding support for new features and improving error handling. The changes include:
try_safe_transfer
andtry_safe_transfer_from
functions that return bool instead of revertingforce_approve
function for handling tokens requiring zero-reset approvaltransfer_and_call_relaxed
,transfer_from_and_call_relaxed
, andapprove_and_call_relaxed
functionssafe_increase_allowance
andsafe_decrease_allowance
functionsResolves #620
PR Checklist