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.

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

feat: ERC-4626 extension #465

Merged
merged 214 commits into from
Jan 30, 2025
Merged

Conversation

Ifechukwudaniel
Copy link
Contributor

@Ifechukwudaniel Ifechukwudaniel commented Dec 19, 2024

Resolves #356

PR Checklist

  • Tests
  • Documentation
  • Changelog

Copy link

netlify bot commented Dec 19, 2024

Deploy Preview for contracts-stylus ready!

Name Link
🔨 Latest commit 07c50e0
🔍 Latest deploy log https://app.netlify.com/sites/contracts-stylus/deploys/679b891b8e089e000836a9f2
😎 Deploy Preview https://deploy-preview-465--contracts-stylus.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Ifechukwudaniel
Copy link
Contributor Author

I just stared this

@0xNeshi
Copy link
Collaborator

0xNeshi commented Dec 19, 2024

I'll convert this to a draft, so that the team does not invest time reviewing a WIP

@0xNeshi 0xNeshi marked this pull request as draft December 19, 2024 07:38
@0xNeshi 0xNeshi self-requested a review December 19, 2024 09:26
@Ifechukwudaniel
Copy link
Contributor Author

@0xNeshi
The Solidity implementation uses safe transfer

SafeERC20.safeTransferFrom(_asset, caller, address(this), assets);

In the Solidity implementation, SafeERC20 is a utility library. However, in the Stylus implementation, SafeTransfer is a contract. What do you think

@0xNeshi
Copy link
Collaborator

0xNeshi commented Dec 19, 2024

@0xNeshi The Solidity implementation uses safe transfer

SafeERC20.safeTransferFrom(_asset, caller, address(this), assets);

In the Solidity implementation, SafeERC20 is a utility library. However, in the Stylus implementation, SafeTransfer is a contract. What do you think

See how VestingWallet handles this

@0xNeshi 0xNeshi marked this pull request as ready for review January 29, 2025 11:14
Copy link
Member

@qalisander qalisander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job guys!
Left a few comments

examples/erc4626/src/lib.rs Show resolved Hide resolved
examples/erc4626/tests/erc4626.rs Outdated Show resolved Hide resolved
@0xNeshi
Copy link
Collaborator

0xNeshi commented Jan 30, 2025

I made decimal_offset part of state, so as to enable devs to override the value. This is similar to how we simulated Solidity's override functionality in Erc721Metadata::_base_uri by adding this state field, which is not part of Solidity version's storage.

Aside from enabling devs to override the decimals offset value, I was able to add additional test cases verifying what happens if this value overflows.

@qalisander qalisander self-requested a review January 30, 2025 11:33
Copy link
Collaborator

@bidzyyys bidzyyys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

@bidzyyys bidzyyys merged commit 5d612e6 into OpenZeppelin:main Jan 30, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: ERC4626 extension
4 participants