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.

Sign up for GitHub

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

Jump to bottom

ref: add associated error types for contract traits #251

Merged
merged 7 commits into from
Aug 21, 2024
Merged

ref: add associated error types for contract traits #251

merged 7 commits into from
Aug 21, 2024

Conversation

qalisander
Copy link
Member

@qalisander qalisander commented Aug 20, 2024 •
edited
Loading

Allows to the library users to reuse our traits with their own error types.
And it is more "rusty" approach having associated error types for traits with fallible functions.
Adds new functional macro for the MethodError trait.

@netlify Netlify
Copy link

netlify bot commented Aug 20, 2024 •
edited
Loading

✅ Deploy Preview for contracts-stylus canceled.

Name Link
🔨 Latest commit 41950bc
🔍 Latest deploy log https://app.netlify.com/sites/contracts-stylus/deploys/66c5f54475e1e2000802d4d3

qalisander
qalisander commented Aug 20, 2024
View reviewed changes
contracts/src/token/erc20/extensions/permit.rs
@@ -60,18 +60,6 @@ pub enum Error {
ECDSA(ecdsa::Error),
}

impl MethodError for erc20::Error {
Copy link
Member Author

Choose a reason for hiding this comment

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

MethodError implementation is moved to the corresponding contract

Copy link
Contributor

Choose a reason for hiding this comment

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

This means that this import can be removed from this file, right?

@codecov Codecov
Copy link

codecov bot commented Aug 20, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 29.16667% with 17 lines in your changes missing coverage. Please review.

Project coverage is 84.1%. Comparing base (375d1d7) to head (41950bc).
Report is 2 commits behind head on main.

Files Patch % Lines
contracts/src/token/erc20/mod.rs 20.0% 8 Missing ⚠️
contracts/src/token/erc721/mod.rs 0.0% 3 Missing ⚠️
contracts/src/utils/cryptography/ecdsa.rs 0.0% 3 Missing ⚠️
contracts/src/utils/structs/checkpoints.rs 0.0% 3 Missing ⚠️
Additional details and impacted files
Files Coverage Δ
contracts/src/token/erc20/extensions/burnable.rs 100.0% <100.0%> (ø)
contracts/src/token/erc20/extensions/permit.rs 0.0% <ø> (ø)
contracts/src/token/erc721/extensions/burnable.rs 100.0% <100.0%> (ø)
...ntracts/src/token/erc721/extensions/consecutive.rs 65.1% <ø> (+0.7%) ⬆️
...ontracts/src/token/erc721/extensions/enumerable.rs 88.2% <100.0%> (ø)
contracts/src/token/erc721/mod.rs 95.7% <0.0%> (-0.3%) ⬇️
contracts/src/utils/cryptography/ecdsa.rs 35.5% <0.0%> (-1.3%) ⬇️
contracts/src/utils/structs/checkpoints.rs 96.5% <0.0%> (-1.1%) ⬇️
contracts/src/token/erc20/mod.rs 94.7% <20.0%> (-1.5%) ⬇️

bidzyyys
bidzyyys reviewed Aug 20, 2024
View reviewed changes
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.

Looks good but I do not like having impl MethodError many times...

@qalisander qalisander force-pushed the error-associated-type-for-contract-traits branch from 42f0502 to bd9049e Compare August 21, 2024 08:09
bidzyyys
bidzyyys approved these changes Aug 21, 2024
View reviewed changes
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.

Looks good!

alexfertel
alexfertel suggested changes Aug 21, 2024
View reviewed changes
Copy link
Contributor

@alexfertel alexfertel left a comment

Choose a reason for hiding this comment

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

If I understand correctly this change, I would expect it to affect our examples, right? What is the practical result of this change?

contracts/src/token/erc20/extensions/permit.rs
@@ -60,18 +60,6 @@ pub enum Error {
ECDSA(ecdsa::Error),
}

impl MethodError for erc20::Error {
Copy link
Contributor

Choose a reason for hiding this comment

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

This means that this import can be removed from this file, right?

contracts/src/token/erc721/extensions/consecutive.rs Show resolved Hide resolved
contracts/src/token/erc721/mod.rs Outdated Show resolved Hide resolved
contracts/src/token/erc721/mod.rs Outdated Show resolved Hide resolved
@qalisander
++
af578e3
@qalisander
Revert "++"
81b42f2 
This reverts commit af578e3.
@qalisander
Revert "++"
6d1d058 
This reverts commit bd9049e.
@qalisander
++
41950bc
@qalisander qalisander merged commit 8f56fb1 into main Aug 21, 2024
21 checks passed
@bidzyyys bidzyyys deleted the error-associated-type-for-contract-traits branch August 23, 2024 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexfertel alexfertel alexfertel approved these changes

@bidzyyys bidzyyys bidzyyys approved these changes

@ggonzalez94 ggonzalez94 Awaiting requested review from ggonzalez94

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@qalisander @alexfertel @bidzyyys