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

Proposal to allow attached_deposit in view contexts #418

Merged
merged 8 commits into from
Jan 27, 2023
102 changes: 102 additions & 0 deletions neps/nep-0418.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
---
NEP: 0418
Title: Remove attached_deposit view panic
Author: Austin Abell <austin.abell@near.org>
DiscussionsTo: https://github.com/nearprotocol/neps/pull/418
Status: Approved
Type: Standards Track
Category: Tools
Version: 1.0.0
Created: 18-Oct-2022
Updated: 27-Jan-2023
---

## Summary

This proposal is to switch the behavior of the `attached_deposit` host function on the runtime from panicking in view contexts to returning 0. This results in a better devX because instead of having to configure an assertion that there was no attached deposit to a function call only for transactions and not view calls, which is impossible because you can send a transaction to any method, you could just apply this assertion without the runtime aborting in view contexts.

## Motivation

This will allow contract SDK frameworks to add the `attached_deposit == 0` assertion for every function on a contract by default. This behavior matches the Solidity/Eth payable modifier and will ensure that funds aren't sent accidentally to a contract in more cases than currently possible.

This can't be done at a contract level because there is no way of checking if a function call is within view context to call `attached_deposit` conditionally. This means that there is no way of restricting the sending of funds to functions intended to be view only because the abort from within `attached_deposit` can't be caught and ignored from inside the contract.

Initial discussion: https://near.zulipchat.com/#narrow/stream/295306-pagoda.2Fcontract-runtime/topic/attached_deposit.20view.20error

## Rationale and alternatives

The rationale for assigning `0u128` to the pointer (`u64`) passed into `attached_deposit` is that it's the least breaking change.

The alternative of returning some special value, say `u128::MAX`, is that it would cause some unintended side effects for view calls using the `attached_deposit`. For example, if `attached_deposit` is called within a function, older versions of a contract that do not check the special value will return a result assuming that the attached deposit is `u128::MAX`. This is not a large concern since it would just be a view call, but it might be a bad UX in some edge cases, where returning 0 wouldn't be an issue.

## Specification

The error inside `attached_deposit` for view calls will be removed, and for all view calls, `0u128` will be set at the pointer passed in.

## Reference Implementation


Currently, the implementation for `attached_deposit` is as follows:
```rust
pub fn attached_deposit(&mut self, balance_ptr: u64) -> Result<()> {
self.gas_counter.pay_base(base)?;

if self.context.is_view() {
return Err(HostError::ProhibitedInView {
method_name: "attached_deposit".to_string(),
}
.into());
}
self.memory_set_u128(balance_ptr, self.context.attached_deposit)
}
```

Which would just remove the check for `is_view` to no longer throw an error:

```rust
pub fn attached_deposit(&mut self, balance_ptr: u64) -> Result<()> {
self.gas_counter.pay_base(base)?;

self.memory_set_u128(balance_ptr, self.context.attached_deposit)
}
```

This assumes that in all cases, `self.context.attached_deposit` is set to 0 in all cases. This can be asserted, or just to be safe, can check if `self.context.is_view()` and set `0u128` explicitly.

## Security Implications

This won't have any implications outside of view calls, so this will not affect anything that is persisted on-chain. This only affects view calls. This can only have a negative side effect if a contract is under the assumption that `attached_deposit` will panic in view contexts. The possibility that this is done _and_ has some value connected with a view call result off-chain seems extremely unlikely.

## Drawbacks

This has a breaking change of the functionality of `attached_deposit` and affects the behavior of some function calls in view contexts if they use `attached_deposit` and no other prohibited host functions.

## Future possibilities

- The Rust SDK, as well as other SDKs, can add the `attached_deposit() == 0` check by default to all methods for safety of use.
- Potentially, other host functions can be allowed where reasonable values can be inferred. For example, `prepaid_gas`, `used_gas` could return 0.

## Decision Context

### 1.0.0 - Initial Version

The initial version of NEP-418 was approved by Tools Working Group members on January 19, 2023 ([meeting recording](https://youtu.be/poVmblmc3L4)).

#### Benefits

- This will allow contract SDK frameworks to add the `attached_deposit == 0` assertion for every function on a contract by default.
- This behavior matches the Solidity/Eth payable modifier and will ensure that funds aren't sent accidentally to a contract in more cases than currently possible.
- Given that there is no way of checking if a function call is within view context to call `attached_deposit` conditionally, this NEP only changes a small surface of the API instead of introducing a new host function.

#### Concerns

| # | Concern | Resolution | Status |
| - | - | - | - |
| 1 | Proposal potentially triggers the protocol version change | It does not trigger the protocol version change. Current update could be considered a client-breaking change update. | Resolved |
| 2 | The contract can assume that `attached_deposit` will panic in view contexts. | The possibility that this is done _and_ has some value connected with a view call result off-chain seems extremely unlikely. | Won't Fix |
| 3 | Can we assume that in all view calls, the `attached_deposit` in the VMContext always zero? | Yes, there is no way to set `attached_deposit` in view calls context | Resolved |

## Copyright
[copyright]: #copyright

Copyright and related rights waived via [CC0](https://creativecommons.org/publicdomain/zero/1.0/).