Private view methods

[agostbiro]

The purpose of this issue is to follow up the conversation from #890 and #1025 on private view methods. I quote here the comments to make it easier to overview. Related issue: https://github.com/near/near-sdk-rs/issues/937.

In #890 @itegulov asked:

Should view functions be allowed to be private? Wouldn't such a function only be callable from a transaction?

In #890 @austinabell replied:

No, even calling env::predecessor_account_id fails in view contexts at runtime. This should be restricted at compile time. Good callout!

In #1025 @agostbiro wrote:

A reviewer in https://github.com/near/near-sdk-rs/pull/890 commented that private view functions shouldn't be allowed. This wasn't implemented in https://github.com/near/near-sdk-rs/pull/890 and when I made the change several tests failed, because methods that take &self as argument are considered ~private~ view. Example failing test case:

#[private] pub fn method(&self, #[callback_unwrap] x: &mut u64, y: String, #[callback_unwrap] z: Vec<u8>) { }

On a related note, at first I was surprised that methods that take self by value are view, but I guess that it makes sense since they cannot mutate the object. As I haven't found a test case for this I added one to make sure that method(self) isn't payable in https://github.com/near/near-sdk-rs/commit/56394ec234587b8195005b8b601c9b80c24b2247.

In #1025 @itegulov replied:

I think you meant that methods that take &self argument are considered view, right? So, as @austinabell mentioned in the original PR, view methods can never be private. That being said, the current near_bindgen API is flawed in that it does not provide clear control of which methods are view or call. There are a couple of ways we could resolve this that we have discussed in the past but never :

1. Make private, payable and other such modifiers automatically mean that this method is call
2. Create #[call] and #[view] modifiers and slowly phase out the old self-based deduction system
3. Just leave everything as is, it has been working so far

In any case, probably out of scope for this PR, just though I would share this here since its related to the work you are doing. I propose we do not prohibit private view functions for now and move this conversation to a separate issue.

[agostbiro]

I did a bit a of testing now with this contract:

#[near_bindgen]
#[derive(BorshDeserialize, BorshSerialize)]
pub struct Contract {
    message: String,
}

#[near_bindgen]
impl Contract {
    pub fn get_greeting(&self) -> String {
        return self.private_get_greeting();
    }

    #[private] pub fn private_get_greeting(&self) -> String {
        return self.message.clone();
    }
}

And I found that I can call get_greeting both with an Account::call and a Contract::view, so maybe it's fine as it is and we don't have to disallow private view methods?

[austinabell]

And I found that I can call get_greeting both with an Account::call and a Contract::view, so maybe it's fine as it is and we don't have to disallow private view methods?

This is because you're not using the #[private] annotation for what it's intended for. The use case is making a cross-contract call from the current contract (commonly used for callbacks). If you call private_get_greeting as a view call (you have to call directly because you can't schedule the call in view context), you'll see it will fail with handler error: [Function call returned an error: wasm execution failed with error: HostError(ProhibitedInView { method_name: "predecessor_account_id" })].

For reference, the code I'm referring to:

    pub fn get_greeting() -> Promise {
        Self::ext(env::current_account_id()).private_get_greeting()
    }

    #[private] pub fn private_get_greeting(&self) -> String {
        "test".to_string()
    }

Where I don't think there is an issue, since these can both just be called in a transaction rather than a view call, but the semantics are a bit muddied because &self is documented as meaning a view call. This just works because view calls can be called as transactions (as there is no view/transaction context passed from the runtime).

[agostbiro]

Thanks for clarifying @austinabell ! I managed to reproduce the error with my code when calling private_get_greeting directly as view, and with the code that you provided by calling get_greeting as view.

I'm still not 100% clear on why the error doesn't occur when calling get_greeting as view in my code. Is the code executed different or is the runtime environment different? Is there anywhere I can read more about this?

Btw while testing this I ran some circles, because I didn't realize that the test:integration command doesn't rebuild the contract, so I'm submitting a PR for that in create-near-app (edit: it's https://github.com/near/create-near-app/pull/2014).

[frol]

I'm still not 100% clear on why the error doesn't occur when calling get_greeting as view in my code. Is the code executed different or is the runtime environment different?

@agostbiro self.private_get_greeting() just calls the function on the struct, but calling view function from RPC, it goes through an onramp created by near_bindgen:

Is there anywhere I can read more about this?

I only trust the source code:

https://github.com/near/near-sdk-rs/blob/e4cb471631c55fb8831403ffea12afc71030abc1/near-sdk-macros/src/core_impl/code_generator/item_impl_info.rs#L36-L52

[agostbiro]

Ah got it, thanks @frol !

So the conclusion seems to me that there are no changes needed regarding private view functions. Did I understand that correctly?

[frol]

@agostbiro Well, given that there is no use for private view functions (they will just fail), it would be great to forbid them at compilation time (#private + &self should be forbidden). Would it be hard to implement? (sounds like a trivial assert after the refactoring, but I don't have the full context in my head at the moment)

[agostbiro]

@frol yeah it's a very small change. I tried it now and the following tests are failing after disallowing private view methods:

Failing examples

https://github.com/near/near-sdk-rs/blob/c39fba338c2fbbb2e4dd1487a4c6c978a780f89c/examples/callback-results/src/lib.rs#L27-L28

https://github.com/near/near-sdk-rs/blob/c39fba338c2fbbb2e4dd1487a4c6c978a780f89c/examples/callback-results/src/lib.rs#L49-L55

https://github.com/near/near-sdk-rs/blob/c39fba338c2fbbb2e4dd1487a4c6c978a780f89c/examples/cross-contract-calls/high-level/src/lib.rs#L26-L27

Failing unit tests

https://github.com/near/near-sdk-rs/blob/c39fba338c2fbbb2e4dd1487a4c6c978a780f89c/near-sdk-macros/src/core_impl/code_generator/item_impl_info.rs#L307

https://github.com/near/near-sdk-rs/blob/c39fba338c2fbbb2e4dd1487a4c6c978a780f89c/near-sdk-macros/src/core_impl/code_generator/item_impl_info.rs#L352

https://github.com/near/near-sdk-rs/blob/c39fba338c2fbbb2e4dd1487a4c6c978a780f89c/near-sdk-macros/src/core_impl/code_generator/item_impl_info.rs#L388

https://github.com/near/near-sdk-rs/blob/c39fba338c2fbbb2e4dd1487a4c6c978a780f89c/near-sdk-macros/src/core_impl/code_generator/item_impl_info.rs#L420

https://github.com/near/near-sdk-rs/blob/c39fba338c2fbbb2e4dd1487a4c6c978a780f89c/near-sdk-macros/src/core_impl/code_generator/item_impl_info.rs#L613

---

With this many tests failing I'm a little concerned about breaking a lot of code in the wild if we disallow private view methods, but if that's unfounded I'm happy to adjust the tests.

[itegulov]

@frol the main issue here is that a lot of contracts in the wild have methods that are view according to their self usage, but not view semantically and are always called in transaction context (@agostbiro provided some examples above). Making this change will break the existing code for them which is why this would have to be a multistep process where we have to decide whether we want to bother with enforcing the view/call unambiguity first.