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: Erc20Wrapper extension #498

Draft
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

Ifechukwudaniel
Copy link
Contributor

@Ifechukwudaniel Ifechukwudaniel commented Jan 16, 2025

Resolves #354

PR Checklist

  • Tests
  • Documentation
  • Changelog

Copy link

netlify bot commented Jan 16, 2025

Deploy Preview for contracts-stylus canceled.

Name Link
🔨 Latest commit 6fd4866
🔍 Latest deploy log https://app.netlify.com/sites/contracts-stylus/deploys/67a3db5ba1a2fb0008da71fc

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 7.29167% with 89 lines in your changes missing coverage. Please review.

Project coverage is 83.3%. Comparing base (1c86069) to head (6fd4866).

Files with missing lines Patch % Lines
contracts/src/token/erc20/extensions/wrapper.rs 7.2% 89 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
contracts/src/token/erc20/extensions/wrapper.rs 7.2% <7.2%> (ø)

@Ifechukwudaniel Ifechukwudaniel marked this pull request as draft January 16, 2025 01:36
@Ifechukwudaniel Ifechukwudaniel changed the title Erc20 wrapper [FEATURE] Erc20Wrapper Jan 24, 2025
@bidzyyys bidzyyys changed the title [FEATURE] Erc20Wrapper feat: Erc20Wrapper extension Jan 27, 2025
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.

Good progess @Ifechukwudaniel, please focus on now:

  • Increasing E2E test coverage
  • Improving Rust docs
  • Solving all clippy warnings.

contracts/src/token/erc20/extensions/wrapper.rs Outdated Show resolved Hide resolved
contracts/src/token/erc20/extensions/wrapper.rs Outdated Show resolved Hide resolved
contracts/src/token/erc20/extensions/wrapper.rs Outdated Show resolved Hide resolved
// ============================================================================

#[e2e::test]
async fn constructs(alice: Account) -> Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Increase E2E test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On this

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.

@Ifechukwudaniel CI does not pass. Please fix it.

CHANGELOG.md Show resolved Hide resolved
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.

Left some comments.

contracts/src/token/erc20/extensions/mod.rs Outdated Show resolved Hide resolved
//! number of "wrapped tokens". This is useful in conjunction with other
//! modules.
//!
//! WARNING: Any mechanism in which the underlying token changes the {balanceOf}
Copy link
Collaborator

@bidzyyys bidzyyys Feb 4, 2025

Choose a reason for hiding this comment

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

Adjust the docs to the current implementation. We do not have balanceOf function.

contracts/src/token/erc20/extensions/wrapper.rs Outdated Show resolved Hide resolved
#[allow(missing_docs)]
error ERC20InvalidUnderlying(address token);

/// Indicates that the address is not an Invalid Sender address.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe there is sth wrong with this docs...

#[allow(missing_docs)]
error ERC20InvalidSender(address sender);

/// Indicates that The address is not a valid Invalid Asset.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe there is sth wrong with this docs...

contracts/src/token/erc20/extensions/wrapper.rs Outdated Show resolved Hide resolved
contracts/src/token/erc20/extensions/wrapper.rs Outdated Show resolved Hide resolved
}
}

// TODO: Add missing tests once `motsu` supports calling external contracts.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should have motsu version that support calling external contracts tomorrow :)
It is already merged into main branch of mostu. You can implement these test cases.

examples/erc20-wrapper/tests/erc20wrapper.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Implement missing E2E test cases. Each scenario must be covered.

@Ifechukwudaniel
Copy link
Contributor Author

Ifechukwudaniel commented Feb 5, 2025

I have been kind of on another project, but I will do my best to round this up asap

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]: ERC20Wrapper extension
2 participants