feat: use proposed `contracttrait` type #350

willemneal · 2025-08-07T18:56:15Z

This also simplifies Upgrade to depend on Ownable.

A continuation of #334

netlify · 2025-08-07T18:56:44Z

✅ Deploy Preview for delightful-dieffenbachia-2097e0 canceled.

Name	Link
🔨 Latest commit	`e322e81`
🔍 Latest deploy log	https://app.netlify.com/projects/delightful-dieffenbachia-2097e0/deploys/68b8afe5fc07120008da0496

…cttrait_pr

ozgunozerk · 2025-09-10T08:21:28Z

I'll post some of the discussion points below that happened through DM. So, that @brozorec and others can chime in.

ozgunozerk · 2025-09-10T08:22:37Z

examples/fungible-allowlist/src/contract.rs

-    type ContractType = AllowList;
+    type Impl = AllowList;


Me:

ContractType is changed to Impl. We decided to keep it as ContractType in our previous discussion

Willem:

My branch of the contracttraits adds the default impl redirect, but uses Impl . What I wanted was to be consistent with contracttraits which used the associated type.

I still think ContractType is more self-explanatory than an abstract name such as Impl. I'd prefer to keep it as ContractType

How about ContractImpl? Type is also broad and it already is a type. What it is is the implementation of the contract so ContractImpl seems appropriate to me.

it is already a type

Yeah I give you that :D

I don't think Impl keyword fits in here.

What it is is the implementation of the contract so ContractImpl seems appropriate to me.

I disagree. It is the associated type. The implementation of the contract is happening elsewhere, this is just the indicator type. I can go with Contract alone, agree that Type is redundant, but I don't think Impl fits here.

ozgunozerk · 2025-09-10T08:23:34Z

examples/fungible-allowlist/src/contract.rs

+
+#[contractimpl]
+impl FungibleBurnable for ExampleContract {
+    type Impl = AllowList;


Me:

Why have we added ContractType to every trait? It was not required back then for FungibleAllowList, FungibleBurnable, AccessControl, etc.

Willem:

The idea was that I wanted an actual default implementation for each. This way devs could replace it with their own and opens up to the trait extensions which I use an another branch.

There are specific reasons that we didn't want to provide default implementations for some modules, like AccessControl. This was a deliberate choice because the functions that will be implemented for the contract are critical w.r.t security, and providing defaults will effectively result in people using the default rather than carefully thinking about their business logic and how to configure AccessControl accordingly.

The reason for the existence of the ContractType trait, is to provide overrides across different types of contracts. For Allowlist, Burnable, AccessControl, there are no overrides needed for their interface.

I'd say this change is contradicting with 2 critical design choices of our library.

ozgunozerk · 2025-09-10T08:24:15Z

examples/fungible-allowlist/src/contract.rs


        // Mint initial supply to the admin
-        Base::mint(e, &admin, initial_supply);
+        Self::internal_mint(e, admin, initial_supply);


Me:

internal_mint is a new addition?

Willem:

mint was public but added an internal_mint to the FT trait with #[internal]. This way if you you wanted to swap of the default implementation it would be guaranteed to have it.

I'm not sure I fully get it, could you elaborate on that a bit more @willemneal ?

ozgunozerk · 2025-09-10T08:25:08Z

examples/nft-royalties/src/test.rs

-    let (receiver, amount) = client.get_royalty_info(&token_id, &1000);
+    let (receiver, amount) = client.royalty_info(&token_id, &1000);


Me:

get_royalty_info is renamed to royalty_info, debatable…

Willem:

It wasn’t renamed because royalty_info was part of the trait and thus exposed already. So there were two functions that did the same thing. So I’m open to renaming.

Yep, that sounds good! This is not a rename for the storage or interface, but a rename to the specific example contract implementation.

ozgunozerk · 2025-09-11T16:41:16Z

packages/tokens/src/fungible/extensions/allowlist/mod.rs

    /// * topics - `["disallow", user: Address]`
    /// * data - `[]`
-    fn disallow_user(e: &Env, user: Address, operator: Address);
+    fn disallow_user(e: &Env, user: &soroban_sdk::Address, operator: &soroban_sdk::Address);


why are we using the full path here &soroban_sdk::Address? Address is already imported above from soroban_sdk?

ozgunozerk · 2025-09-11T16:44:01Z

packages/tokens/src/non_fungible/extensions/enumerable/mod.rs

-pub trait NonFungibleEnumerable: NonFungibleToken<ContractType = Enumerable> {
+#[contracttrait(add_impl_type = true)]
+pub trait NonFungibleEnumerable {


does it still work in mutually exclusive way with Consecutive for example?

What I mean is, trait bound is removed: pub trait NonFungibleEnumerable: NonFungibleToken<ContractType = Enumerable>

and, this trait bound was allowing us to get mutual exclusivity across incompatible contract types. More concretely, one cannot implement both Enumerable and Consecutive for the same contract.

brozorec

@willemneal great work 👌 This approach is viable and I think it makes the library more ergonomic.

There is the question about the unimplemented function (cf. one of the comments) that's a blocker for me atm.

brozorec · 2025-09-11T14:47:37Z

packages/access/src/access_control/mod.rs

    /// * topics - `["role_granted", role: Symbol, account: Address]`
    /// * data - `[caller: Address]`
-    fn grant_role(e: &Env, caller: Address, account: Address, role: Symbol);
+    fn grant_role(e: &Env, caller: &Address, account: &Address, role: &soroban_sdk::Symbol);


Why some of the params are fully-qualified and others are not? What's the benefit?

The contracttrait macro will generate the code and this basically means copy/pasting the types. So without this it means you will need to import them. Which isn't too bad but could be confusing for devs.

brozorec · 2025-09-11T14:54:26Z

packages/access/src/access_control/storage.rs

+    fn enforce_admin_auth(e: &Env) {
+        let Some(admin) = Self::get_admin(e) else {
+            soroban_sdk::panic_with_error!(e, AccessControlError::AdminNotSet);
+        };
+        admin.require_auth();
+    }
+
+    fn ensure_role(e: &Env, caller: &soroban_sdk::Address, role: &soroban_sdk::Symbol) {
+        if Self::has_role(e, caller, role).is_none() {
+            soroban_sdk::panic_with_error!(e, AccessControlError::Unauthorized);
+        }
+    }
+
+    fn ensure_if_admin_or_admin_role(
+        e: &Env,
+        caller: &soroban_sdk::Address,
+        role: &soroban_sdk::Symbol,
+    ) {
+        let is_admin = match Self::get_admin(e) {
+            Some(admin) => caller == &admin,
+            None => false,
+        };
+        let is_admin_role = match Self::get_role_admin(e, role) {
+            Some(admin_role) => Self::has_role(e, caller, &admin_role).is_some(),
+            None => false,
+        };
+        if !is_admin && !is_admin_role {
+            soroban_sdk::panic_with_error!(e, AccessControlError::Unauthorized);
+        }
+    }


If there are default implementations for those internals, why do we re-impl them here?

brozorec · 2025-09-11T15:06:17Z

packages/access/src/ownable/mod.rs

+    #[internal]
+    fn only_owner(e: &soroban_sdk::Env) {
+        let Some(owner) = Self::get_owner(e) else {
+            panic_with_error!(e, OwnableError::OwnerNotSet);
+        };
+        owner.require_auth()
+    }
+
+    #[internal]
+    fn enforce_owner_auth(e: &soroban_sdk::Env) -> Address {
+        let Some(owner) = Self::get_owner(e) else {
+            panic_with_error!(e, OwnableError::OwnerNotSet);
+        };
+        owner.require_auth();
+        owner
+    }


maybe we don't need them both

brozorec · 2025-09-12T08:55:42Z

packages/tokens/src/fungible/extensions/allowlist/storage.rs

+    fn allow_user(e: &Env, user: &Address, _operator: &Address) {
+        Self::allow_user_no_auth(e, user);
+    }


I commented out this fn and also the fn in the fungible-allowlist example + the FungibleAllowList trait in mod.rs doesn't provide a default impl, but everything still compiles 🤔

I was expecting it would scream about the unimplemented function, but that's not the case, which can be a serious issue for sensitive functions like this one.

feat: use newest soroban PR

56f0362

willemneal force-pushed the feat/use_contracttrait_pr branch from 746fa43 to 56f0362 Compare August 8, 2025 18:02

willemneal added 11 commits August 8, 2025 17:06

fix: update to changes upstream

7bfbf45

fix: type AccessControler -> AccessController

708b79b

fix: typo -> Consecutive

0549f3b

fix: fmt

650920c

fix: use default impl for allow/blocklist

c378927

fix: revert test and fix clippy

aa6bde6

fix: AccessControlError typo

2753916

fix: simplify tests

80a7d3b

fix: enumeration and consectutive

5880dff

fix: return to original imports

f98d0a4

fix: clean up

4d8287e

willemneal force-pushed the feat/use_contracttrait_pr branch from 5d8bb5a to 2869014 Compare August 12, 2025 15:43

fix: revert upgrade

9bc9170

willemneal force-pushed the feat/use_contracttrait_pr branch from 2869014 to 9bc9170 Compare August 12, 2025 17:14

fix: deps

20ff3ec

willemneal marked this pull request as ready for review August 12, 2025 21:49

willemneal added 2 commits August 21, 2025 10:10

fix: update to newest sdk

ef977bc

fix: ignore deprecation until we can rewrite events

756a790

willemneal mentioned this pull request Aug 21, 2025

Initial Suggestion for a organizing into one crate #354

Open

Merge remote-tracking branch 'OpenZeppelin/main' into feat/use_contra…

e322e81

…cttrait_pr

ozgunozerk reviewed Sep 10, 2025

View reviewed changes

ozgunozerk reviewed Sep 11, 2025

View reviewed changes

brozorec reviewed Sep 12, 2025

View reviewed changes

		let (receiver, amount) = client.get_royalty_info(&token_id, &1000);
		let (receiver, amount) = client.royalty_info(&token_id, &1000);

feat: use proposed contracttrait type #350

Are you sure you want to change the base?

feat: use proposed contracttrait type #350

Uh oh!

Conversation

willemneal commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for delightful-dieffenbachia-2097e0 canceled.

Uh oh!

ozgunozerk commented Sep 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brozorec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

feat: use proposed `contracttrait` type #350

feat: use proposed `contracttrait` type #350

willemneal commented Aug 7, 2025 •

edited

Loading

netlify bot commented Aug 7, 2025 •

edited

Loading