Skip to content

Conversation

@chadoh
Copy link
Member

@chadoh chadoh commented May 14, 2025

@chadoh chadoh force-pushed the feat/better-contracts branch 2 times, most recently from 741ef02 to a7e3526 Compare May 14, 2025 16:19
Base automatically changed from feat/better-contracts to main May 14, 2025 18:06
@chadoh chadoh force-pushed the feat/oz branch 2 times, most recently from b9805cd to 495edac Compare May 15, 2025 18:41
@chadoh chadoh force-pushed the feat/oz branch 2 times, most recently from d68ca62 to 88ba1cd Compare May 23, 2025 20:07
@chadoh chadoh marked this pull request as ready for review May 27, 2025 18:57
Comment on lines +66 to +78
let owner: Address = e.storage().instance().get(&OWNER).expect("owner should be set");
if owner != caller {
panic_with_error!(e, ExampleContractError::Unauthorized);
}

pausable::pause(e, &caller);
}

fn unpause(e: &Env, caller: Address) {
// When `ownable` module is available,
// the following checks should be equivalent to:
// `ownable::only_owner(&e);`
let owner: Address = e.storage().instance().get(&OWNER).expect("owner should be set");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let owner: Address = e.storage().instance().get(&OWNER).expect("owner should be set");
if owner != caller {
panic_with_error!(e, ExampleContractError::Unauthorized);
}
pausable::pause(e, &caller);
}
fn unpause(e: &Env, caller: Address) {
// When `ownable` module is available,
// the following checks should be equivalent to:
// `ownable::only_owner(&e);`
let owner: Address = e.storage().instance().get(&OWNER).expect("owner should be set");
let owner: Address = e.storage().instance().get(&OWNER).unwrap_optimized();
if owner != caller {
panic_with_error!(e, ExampleContractError::Unauthorized);
}
pausable::pause(e, &caller);
}
fn unpause(e: &Env, caller: Address) {
// When `ownable` module is available,
// the following checks should be equivalent to:
// `ownable::only_owner(&e);`
let owner: Address = e.storage().instance().get(&OWNER)..unwrap_optimized();

Since the contract can't be deployed with setting the owner it's probably better to use this version of unwrap as it's provided by the soroban-sdk and is well, optimized.

I'm also confused why we need to pass the caller. Couldn't we just do owner.require_auth()? It seems like an extra unneeded check here.

This is something not worth slowing things down here but I think that it is probably because the authors assumed you needed to pass the account.

The CLI currently has a bug for auth where it only considers accounts that were passed in rather than any account referenced in the auth entries provided by simulation. So perhaps they were using the CLI and found that they needed to pass the account to have auth work correctly.

I'll open an issue there about it and with OZ.

Copy link
Member Author

Choose a reason for hiding this comment

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

This contract is copied directly over from their examples. Can you send them a PR or some comments upstream?

@chadoh chadoh merged commit 391fdef into main May 30, 2025
1 check passed
@chadoh chadoh deleted the feat/oz branch May 30, 2025 16:48
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.

Newly-initialized Scaffold Stellar projects will start with at least two example OpenZeppelin contracts

4 participants