From 4954f25029a01fd71f53cced6b36268d912dc78b Mon Sep 17 00:00:00 2001 From: Austin Abell Date: Wed, 19 Oct 2022 16:41:30 -0400 Subject: [PATCH 1/8] Proposal to allow attached_deposit in view contexts --- neps/nep-xxxx.md | 90 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 neps/nep-xxxx.md diff --git a/neps/nep-xxxx.md b/neps/nep-xxxx.md new file mode 100644 index 000000000..d5d61585c --- /dev/null +++ b/neps/nep-xxxx.md @@ -0,0 +1,90 @@ +--- +NEP: TODO +Title: Remove attached_deposit view panic +Author: Austin Abell +DiscussionsTo: https://github.com/nearprotocol/neps/pull/0000 +Status: Draft +Type: Standards Track +Category: Contract +Created: 18-Oct-2022 +--- + +## 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 + +After the protocol version, the error inside `attached_deposit` will be skipped, and for all view calls, `0u128` will be set at the pointer passed in. + +## Reference Implementation (Required for Protocol Working Group proposals, optional for other categories) + + +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 have to add the check: + +```rust +pub fn attached_deposit(&mut self, balance_ptr: u64) -> Result<()> { + self.gas_counter.pay_base(base)?; + + if self.context.protocol_version < VIEW_ATTACHED_DEPOSIT_PROTOCOL_VERSION && 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) +} +``` + +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 (Optional) + +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 (Optional) + +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. + +## Unresolved Issues (Optional) + +- Is the assumption that in all view calls, the `attached_deposit` in the VMContext is zero correct? + +## 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. + +## Copyright +[copyright]: #copyright + +Copyright and related rights waived via [CC0](https://creativecommons.org/publicdomain/zero/1.0/). From 68451f427501900ac986056d1169f4d70019553d Mon Sep 17 00:00:00 2001 From: Austin Abell Date: Wed, 19 Oct 2022 16:42:56 -0400 Subject: [PATCH 2/8] update nep # --- neps/{nep-xxxx.md => nep-0418.md} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename neps/{nep-xxxx.md => nep-0418.md} (98%) diff --git a/neps/nep-xxxx.md b/neps/nep-0418.md similarity index 98% rename from neps/nep-xxxx.md rename to neps/nep-0418.md index d5d61585c..f393d4da6 100644 --- a/neps/nep-xxxx.md +++ b/neps/nep-0418.md @@ -1,8 +1,8 @@ --- -NEP: TODO +NEP: 0418 Title: Remove attached_deposit view panic Author: Austin Abell -DiscussionsTo: https://github.com/nearprotocol/neps/pull/0000 +DiscussionsTo: https://github.com/nearprotocol/neps/pull/418 Status: Draft Type: Standards Track Category: Contract From f46fc5ce20068eb2af1812bdc4366af373b6837f Mon Sep 17 00:00:00 2001 From: Austin Abell Date: Fri, 21 Oct 2022 15:05:01 -0400 Subject: [PATCH 3/8] Update neps/nep-0418.md Co-authored-by: Vlad Frolov --- neps/nep-0418.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neps/nep-0418.md b/neps/nep-0418.md index f393d4da6..aa356cb11 100644 --- a/neps/nep-0418.md +++ b/neps/nep-0418.md @@ -31,7 +31,7 @@ The alternative of returning some special value, say `u128::MAX`, is that it wou After the protocol version, the error inside `attached_deposit` will be skipped, and for all view calls, `0u128` will be set at the pointer passed in. -## Reference Implementation (Required for Protocol Working Group proposals, optional for other categories) +## Reference Implementation Currently, the implementation for `attached_deposit` is as follows: From e5844ae28295dbf0315dcea9a447d2103779f931 Mon Sep 17 00:00:00 2001 From: Austin Abell Date: Wed, 26 Oct 2022 10:31:01 -0400 Subject: [PATCH 4/8] change spec to not change based on protocol version --- neps/nep-0418.md | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/neps/nep-0418.md b/neps/nep-0418.md index aa356cb11..78f3cde92 100644 --- a/neps/nep-0418.md +++ b/neps/nep-0418.md @@ -29,7 +29,7 @@ The alternative of returning some special value, say `u128::MAX`, is that it wou ## Specification -After the protocol version, the error inside `attached_deposit` will be skipped, and for all view calls, `0u128` will be set at the pointer passed in. +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 @@ -49,18 +49,12 @@ pub fn attached_deposit(&mut self, balance_ptr: u64) -> Result<()> { } ``` -Which would just have to add the check: +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)?; - if self.context.protocol_version < VIEW_ATTACHED_DEPOSIT_PROTOCOL_VERSION && 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) } ``` From 5c71fad77d7ddb57e0661174a365c5e96c8aa4ea Mon Sep 17 00:00:00 2001 From: Vlad Frolov Date: Fri, 27 Jan 2023 12:30:08 +0100 Subject: [PATCH 5/8] chore: Added Decision Context to the NEP-418 --- neps/nep-0418.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/neps/nep-0418.md b/neps/nep-0418.md index 78f3cde92..06f20028d 100644 --- a/neps/nep-0418.md +++ b/neps/nep-0418.md @@ -78,6 +78,26 @@ This has a breaking change of the functionality of `attached_deposit` and affect - 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 From 8ec5d3960e2aa261c79772844ffb40feaa1609b1 Mon Sep 17 00:00:00 2001 From: Vlad Frolov Date: Fri, 27 Jan 2023 12:31:36 +0100 Subject: [PATCH 6/8] chore: Updated the metadata of NEP-418 --- neps/nep-0418.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/neps/nep-0418.md b/neps/nep-0418.md index 06f20028d..d89df1040 100644 --- a/neps/nep-0418.md +++ b/neps/nep-0418.md @@ -3,10 +3,12 @@ NEP: 0418 Title: Remove attached_deposit view panic Author: Austin Abell DiscussionsTo: https://github.com/nearprotocol/neps/pull/418 -Status: Draft +Status: Approved Type: Standards Track -Category: Contract +Category: Tools +Version: 1.0.0 Created: 18-Oct-2022 +Updated: 27-Jan-2023 --- ## Summary From 336bce52134b3296f3fffff731643f29da7ff805 Mon Sep 17 00:00:00 2001 From: Vlad Frolov Date: Fri, 27 Jan 2023 12:34:26 +0100 Subject: [PATCH 7/8] chore: Removed 'Unresolved Issues' section from NEP-418 as it was addressed in the review and mentioned in the Decision Context section --- neps/nep-0418.md | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/neps/nep-0418.md b/neps/nep-0418.md index d89df1040..8455feb7b 100644 --- a/neps/nep-0418.md +++ b/neps/nep-0418.md @@ -63,18 +63,14 @@ pub fn attached_deposit(&mut self, balance_ptr: u64) -> Result<()> { 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 (Optional) +## 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 (Optional) +## 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. -## Unresolved Issues (Optional) - -- Is the assumption that in all view calls, the `attached_deposit` in the VMContext is zero correct? - ## 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. From ae75c95d4f23923b5d3059bae9fe3fd65510a7cb Mon Sep 17 00:00:00 2001 From: Vlad Frolov Date: Fri, 27 Jan 2023 12:37:53 +0100 Subject: [PATCH 8/8] chore: Fixed code indentation (replaced tabs with spaces) in NEP-418 --- neps/nep-0418.md | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/neps/nep-0418.md b/neps/nep-0418.md index 8455feb7b..d6788a155 100644 --- a/neps/nep-0418.md +++ b/neps/nep-0418.md @@ -39,15 +39,15 @@ The error inside `attached_deposit` for view calls will be removed, and for all 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) + 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) } ``` @@ -55,9 +55,9 @@ 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.gas_counter.pay_base(base)?; - self.memory_set_u128(balance_ptr, self.context.attached_deposit) + self.memory_set_u128(balance_ptr, self.context.attached_deposit) } ```