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

contract_ref! produces traits with too strong requirements #1868

Open
deuszx opened this issue Aug 7, 2023 · 2 comments
Open

contract_ref! produces traits with too strong requirements #1868

deuszx opened this issue Aug 7, 2023 · 2 comments
Assignees
Labels
A-ink_lang [ink_lang] Work item B-feature-request A request for a new feature.

Comments

@deuszx
Copy link

deuszx commented Aug 7, 2023

Since contract_ref! macro produces the "access trait" with the exact same method signatures as the ones on the original contract, sometimes it puts too strong of requirements on self on the caller's side.

Consider the following case:

let mut psp22_ref: ink::contract_ref!(PSP22) = (*reward_token).into();
let balance: Balance = psp22_ref.balance_of(Self::env().account_id());
if balance > 0 {
  psp22_ref.transfer(running.owner, balance)?;
}

compiler requires that psp22_ref is mut because PSP22::transfer(&mut self, ...). Even though it should not be necessary on the call side since psp22_ref is just an address and not an actual contract state.

@SkymanOne SkymanOne added A-ink_lang [ink_lang] Work item B-feature-request A request for a new feature. labels Aug 7, 2023
@xgreenx
Copy link
Collaborator

xgreenx commented Aug 8, 2023

I agree that we don't need to require a mut ownership level because, under the hood, we call a host function that behaves similarly for "read" and "write" methods.

But on the other side, it adds clarity to actions. When you call the mut method, you know it would change things on the callee side. It is not a problem to use ink::contract_ref! in the methods because you can always add mut to the variables showing that you want to modify the internal state of the callee object.

The problem comes when you want to store ink::contract_ref! as a field of your contract. Calling transfer in your case will force your message method also to be &mut self. You have two options here to avoid &mut self: work with AccountId and create ink::contract_ref! on demand; copy this variable with clone and mutate the copy.

It is possible to alter the trait/method by the ink macro during definition and replace all &mut self with &self. But it will affect every part of the code generation and add a "magic" behavior to each method.

I prefer to avoid that and keep things simple in a Rust way. A clone of the AccountId is a cheap operation, but you follow all rules defined by the Rust and don't have any unusual behavior (a.k.a magic).

@deuszx
Copy link
Author

deuszx commented Aug 18, 2023

I don't think the knowledge of whether the method mutates the called contract or not is useful in more ways than simply theoretical. I doubt SC authors care about that. Even if the call is potentially dangerous (reentrancy attack) it can still be read-only (non-mutable). SC calling is too different from "normal Rust" for it to matter IMHO.

If you think it's too much work or would add too much "magic" (and/or gotchas) then these arguments convince me more than the one about having assumptions in the type-system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ink_lang [ink_lang] Work item B-feature-request A request for a new feature.
Projects
None yet
Development

No branches or pull requests

3 participants