fix!(vm-logic): Remove error from attached deposit for view calls #7936
Conversation
| use near_primitives::config::{VMLimitConfig, ViewConfig}; | ||
|
|
There was a problem hiding this comment.
| use near_primitives::config::{VMLimitConfig, ViewConfig}; | |
| use near_primitives::config::{VMLimitConfig, ViewConfig}; |
| context.view_config = | ||
| Some(ViewConfig { max_gas_burnt: VMLimitConfig::test().max_gas_burnt }); | ||
| context.account_balance = 0; | ||
| context.attached_deposit = amount; |
There was a problem hiding this comment.
This is a bit o of a self-defeting tests. I think what we want to check here is that, for view calls, we indeed set the deposit to zero. I don't know off the top of my head where such a test should live at, could take a look once we are sure we want this.
There was a problem hiding this comment.
This test just checks that the attached deposit is returned based on context with view config set (this test fails on master).
The assertion of "attached deposit must always be zero with view config context" seemed intuitively out of scope of this PR. If there was some test setup that I missed that would allow checking this, happy to make changes, but it seems most unit tests ignore this variant and construct contexts with a deposit in view context.
Question (to anyone): will there ever be a use case to configure the context for views, or do we want this to always return zero in view?
Alternatively could just statically return 0 in view contexts for attached_deposit if the latter is true and we want to be safe
|
I don't have privileges to push to this branch anymore, but I have updated it locally. Should I re-open this PR under my personal domain fork, or is something else preferred here? I pushed the changes here: https://github.com/austinabell/nearcore/tree/austin/ad_view if it's easier for someone to just push that here |
|
Hey @austinabell. I propose that you close this PR and create a new one following the suggestions laid out in https://github.com/near/nearcore/blob/master/CONTRIBUTING.md#pull-requests. Essentially, you create a new PR using your fork. That way, you will not have to rely on someone from Pagoda to help manage the commits. |
|
Closed for #8433 |
) Description copied from #7936 Accompanying PR for near/NEPs#418 It's very simple, so I figured I'd just open. I'm generally uncomfortable with how certain context is redundant for view context, and it wasn't clear whether or not this check should be kept and just return 0 for attached deposit for view contexts. Seems like the only logic which uses the view logic [sets the deposit to 0](https://github.com/near/nearcore/blob/d13f66bcdf71ff6a6fdf9f31edad1182283a7523/runtime/runtime/src/state_viewer/mod.rs#L224), but I'm not sure if it will ever be wanted to be able to configure context for view calls in the future.
…ar#8433) Description copied from near#7936 Accompanying PR for near/NEPs#418 It's very simple, so I figured I'd just open. I'm generally uncomfortable with how certain context is redundant for view context, and it wasn't clear whether or not this check should be kept and just return 0 for attached deposit for view contexts. Seems like the only logic which uses the view logic [sets the deposit to 0](https://github.com/near/nearcore/blob/d13f66bcdf71ff6a6fdf9f31edad1182283a7523/runtime/runtime/src/state_viewer/mod.rs#L224), but I'm not sure if it will ever be wanted to be able to configure context for view calls in the future.
…ar#8433) Description copied from near#7936 Accompanying PR for near/NEPs#418 It's very simple, so I figured I'd just open. I'm generally uncomfortable with how certain context is redundant for view context, and it wasn't clear whether or not this check should be kept and just return 0 for attached deposit for view contexts. Seems like the only logic which uses the view logic [sets the deposit to 0](https://github.com/near/nearcore/blob/d13f66bcdf71ff6a6fdf9f31edad1182283a7523/runtime/runtime/src/state_viewer/mod.rs#L224), but I'm not sure if it will ever be wanted to be able to configure context for view calls in the future.
Accompanying PR for near/NEPs#418
It's very simple, so I figured I'd just open.
I'm generally uncomfortable with how certain context is redundant for view context, and it wasn't clear whether or not this check should be kept and just return 0 for attached deposit for view contexts. Seems like the only logic which uses the view logic sets the deposit to 0, but I'm not sure if it will ever be wanted to be able to configure context for view calls in the future.