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: #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 #890 commented that private view functions shouldn't be allowed. This wasn't implemented in #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 56394ec.

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 near/create-near-app#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:

near-sdk-rs/near-sdk-macros/src/core_impl/code_generator/item_impl_info.rs

Lines 36 to 52 in e4cb471

	#[test]
	fn trait_implt() {
	let impl_type: Type = syn::parse_str("Hello").unwrap();
	let mut method: ImplItemMethod = syn::parse_str("fn method(&self) { }").unwrap();
	let method_info = ImplItemMethodInfo::new(&mut method, true, impl_type).unwrap().unwrap();
	let actual = method_info.method_wrapper();
	let expected = quote!(
	#[cfg(target_arch = "wasm32")]
	#[no_mangle]
	pub extern "C" fn method() {
	near_sdk::env::setup_panic_hook();
	let contract: Hello = near_sdk::env::state_read().unwrap_or_default();
	contract.method();
	}
	);
	assert_eq!(expected.to_string(), actual.to_string());
	}

[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

near-sdk-rs/examples/callback-results/src/lib.rs

Lines 27 to 28 in c39fba3

	#[private]
	pub fn b(fail: bool) -> &'static str {

near-sdk-rs/examples/callback-results/src/lib.rs

Lines 49 to 55 in c39fba3

	#[private]
	pub fn handle_callbacks(
	#[callback_unwrap] a: u8,
	#[callback_result] b: Result<String, PromiseError>,
	#[callback_result] c: Result<u8, PromiseError>,
	#[callback_result] d: Result<(), PromiseError>,
	) -> (bool, bool, bool) {

near-sdk-rs/examples/cross-contract-calls/high-level/src/lib.rs

Lines 26 to 27 in c39fba3

	#[private]
	pub fn factorial_mult(&self, n: u32, #[callback_unwrap] cur: u32) -> u32 {

Failing unit tests

near-sdk-rs/near-sdk-macros/src/core_impl/code_generator/item_impl_info.rs

Line 307 in c39fba3

let mut method: ImplItemMethod = parse_quote! {

near-sdk-rs/near-sdk-macros/src/core_impl/code_generator/item_impl_info.rs

Line 352 in c39fba3

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

near-sdk-rs/near-sdk-macros/src/core_impl/code_generator/item_impl_info.rs

Line 388 in c39fba3

#[private] pub fn method(&self, #[callback_result] x: &mut Result<u64, PromiseError>, #[callback_result] y: Result<String, PromiseError>) { }

near-sdk-rs/near-sdk-macros/src/core_impl/code_generator/item_impl_info.rs

Line 420 in c39fba3

#[private] pub fn method(&self, #[callback_vec] x: Vec<String>, y: String) { }

near-sdk-rs/near-sdk-macros/src/core_impl/code_generator/item_impl_info.rs

Line 613 in c39fba3

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

---

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.