Skip to content

feat: update non-token contracts to SDK v0.9#659

Merged
0xNeshi merged 42 commits intosdk-v0.9-and-ctorsfrom
sdk-v0.9-and-ctors-non-token
May 19, 2025
Merged

feat: update non-token contracts to SDK v0.9#659
0xNeshi merged 42 commits intosdk-v0.9-and-ctorsfrom
sdk-v0.9-and-ctors-non-token

Conversation

@0xNeshi
Copy link
Collaborator

@0xNeshi 0xNeshi commented May 15, 2025

No description provided.

@0xNeshi 0xNeshi self-assigned this May 15, 2025
@0xNeshi 0xNeshi changed the title Sdk v0.9 and ctors non token feat: update non-token contracts to SDK v0.9 May 15, 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.

First pass

}

#[public]
#[implements(IOwnable2Step<Error = ownable::Error>, IErc165)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on Solidity Ownable2Step is Ownable and with some features.
I think we should implement both IOwnable and change IOwnable2Step.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v5.3.0/contracts/access/Ownable2Step.sol

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO you should remove all the fns from IOwnable2Step trait that exist in IOwnable trait and apply #[implements(IOwnable2Step<Error = ownable::Error>, IOwnable<Error = ownable::Error>, IErc165)]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the same situation as for IVestingWallet:

The problem with that is that devs who inherit IVestingWallet might forget to inherit IOwnable too.

What you suggested would be the right approach if it were possible to define:

pub trait IVestingWallet: IOwnable

Until the above is possible, IOwnable2Step should include IOwnable functions


#[public]
#[inherit(Erc20, Ownable2Step)]
#[implements(IErc20<Error = Error>, IOwnable2Step<Error = Error>)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on my previous comment, it should implement also IOwnable trait.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should do that once IOwnable2Step can be defined as:

pub trait IOwnable2Step: IOwnable {
      // we should remove IOwnable function definitions from this trait now
}

Until then, we have this issue

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should implement both IOwnable and IOwnable2Step traits. You should remove fns from IOwnable2Step that exist in IOwnable trait IMHO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What is there to be gained by it? It would just duplicate functions existing in IVestingWallet.
It will make sense to implement IOwnable once this is addressed

from: Address,
to: Address,
value: U256,
) -> Result<bool, <Self as IErc20>::Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not using just Error?
Applies to all examples.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It works as per your suggestion.

I was just playing around, reverted all <Self as Interface>::Error back to Self::Error - cargo stylus export-abi and cargo check --features export-abi now works 🤔

Copy link
Collaborator Author

@0xNeshi 0xNeshi May 15, 2025

Choose a reason for hiding this comment

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

We should check if we can revert this in traits before final merge into branch v0.2


#[public]
#[inherit(VestingWallet)]
#[implements(IVestingWallet<Error = vesting_wallet::Error>)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on my previous comment, it should implement also IOwnable trait.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like I said before

Copy link
Collaborator Author

@0xNeshi 0xNeshi May 16, 2025

Choose a reason for hiding this comment

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

What is there to be gained by it? It would just duplicate functions existing in IVestingWallet.
It will make sense to implement IOwnable once this is addressed

Base automatically changed from sdk-v0.9-and-ctors-erc20 to sdk-v0.9-and-ctors May 15, 2025 18:40
@0xNeshi 0xNeshi marked this pull request as ready for review May 15, 2025 19:16
@0xNeshi 0xNeshi requested a review from qalisander as a code owner May 15, 2025 19:16
@0xNeshi 0xNeshi requested a review from bidzyyys May 15, 2025 19:16
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.

Almost ready 🚀


#[public]
#[inherit(Erc20, Ownable2Step)]
#[implements(IErc20<Error = Error>, IOwnable2Step<Error = Error>)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should implement both IOwnable and IOwnable2Step traits. You should remove fns from IOwnable2Step that exist in IOwnable trait IMHO.


#[public]
#[inherit(VestingWallet)]
#[implements(IVestingWallet<Error = vesting_wallet::Error>)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like I said before

@bidzyyys bidzyyys self-requested a review May 16, 2025 08:54
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 an approval in advance.

@codecov
Copy link

codecov bot commented May 16, 2025

Codecov Report

Attention: Patch coverage is 60.00000% with 30 lines in your changes missing coverage. Please review.

Please upload report for BASE (sdk-v0.9-and-ctors@24e93b7). Learn more about missing BASE report.
Report is 1 commits behind head on sdk-v0.9-and-ctors.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
contracts/src/finance/vesting_wallet.rs 41.6% 14 Missing ⚠️
contracts/src/access/ownable.rs 55.0% 9 Missing ⚠️
contracts/src/access/ownable_two_step.rs 68.4% 6 Missing ⚠️
...ntracts/src/token/erc721/extensions/consecutive.rs 50.0% 1 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
contracts/src/access/control.rs 98.6% <100.0%> (ø)
contracts/src/token/common/erc2981.rs 99.2% <100.0%> (ø)
contracts/src/token/erc1155/extensions/supply.rs 80.7% <ø> (ø)
contracts/src/token/erc20/extensions/capped.rs 50.0% <ø> (ø)
contracts/src/token/erc20/extensions/erc4626.rs 18.7% <100.0%> (ø)
contracts/src/token/erc20/extensions/flash_mint.rs 76.4% <100.0%> (ø)
contracts/src/token/erc20/extensions/metadata.rs 61.2% <100.0%> (ø)
contracts/src/token/erc20/extensions/wrapper.rs 92.4% <100.0%> (ø)
contracts/src/token/erc20/mod.rs 97.2% <100.0%> (ø)
contracts/src/token/erc20/utils/safe_erc20.rs 53.1% <100.0%> (ø)
... and 8 more

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.

Good job!

@0xNeshi 0xNeshi force-pushed the sdk-v0.9-and-ctors-non-token branch from a57031f to 5b64a55 Compare May 16, 2025 14:39
@0xNeshi 0xNeshi merged commit 5d84b1f into sdk-v0.9-and-ctors May 19, 2025
16 of 17 checks passed
@0xNeshi 0xNeshi deleted the sdk-v0.9-and-ctors-non-token branch May 19, 2025 09:09
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.

3 participants