Proposal to allow attached_deposit in view contexts #418

austinabell · 2022-10-19T20:41:59Z

A small UX improvement for contract developers. Markdown https://github.com/near/NEPs/blob/austin/view_attached_deposit/neps/nep-0418.md

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

render · 2022-10-19T20:42:05Z

Your Render PR Server URL is https://nomicon-pr-418.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-cd8626irrk04vs7clvb0.

mfornet · 2022-10-20T08:23:41Z

I support this proposal. It can probably be extended to other methods like predecessor_account_id, where some sensible value can be returned. We were having a similar issue in aurora-engine.

Btw, I don't think this requires a protocol version upgrade, given this only changes the behavior of view methods which can be done today by some clients without any impact on the network.

frol · 2022-10-21T18:28:11Z

neps/nep-0418.md

+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() {


As @mfornet mentioned, it seems that this proposal does not necessarily trigger a protocol change, which lowers the barrier for this NEP to be approved, I guess.

Is that not a side effect we want to avoid? ie someone querying a function at a historical block and it giving inconsistent results back before and after updating this? Do we not have any guarantees about this?

This could be considered a client-breaking change update, but it isn't a protocol upgrade. If there were more than one client out there, it could be possible that some clients would implement these features in different ways.

I understand that making this change could break some assumptions about the code that has been written already that relies on current behavior. But even if you do a Protocol Upgrade, it won't solve the issue for them (users can't pin an older protocol version).

IMO we should use the proposed behavior for legacy blocks as well, but I understand that if this means breaking too many apps, it is unacceptable. In that case, we can apply this behavior after some blocks. Even if the choice is to use this strategy after some fixed block, I still think no Protocol Version update is required (just the client version).

hmm, I guess my intuition was that view semantics were built into the protocol. I guess that's technically not the case.

I'll change the proposal to not change behaviour based on network upgrade, and we can evaluate and try to minimize the risk of this known unknown and think of other potential risks.

frol · 2022-10-21T18:30:57Z

@near/wg-protocol I reviewed this NEP as a moderator and it looks good. Please, review and cast your decision in the comments, and let @near/nep-moderators know when you want to move forward to the voting stage.

neps/nep-0418.md

frol · 2022-10-21T18:32:50Z

@mfornet

It can probably be extended to other methods like predecessor_account_id, where some sensible value can be returned.

I cannot think of a sensible value for predecessor_account_id given it should return a valid AccountId.

Co-authored-by: Vlad Frolov <frolvlad@gmail.com>

mfornet · 2022-10-22T17:34:37Z

@mfornet

It can probably be extended to other methods like predecessor_account_id, where some sensible value can be returned.

I cannot think of a sensible value for predecessor_account_id given it should return a valid AccountId.

One suggestion is to use system, and the other is to allow the user to pass the predecessor_account_id as an argument to the view call (no signatures required).

Outside of the scope of this PR but still relevant:

Ideally, we should be able to do dry runs of change-calls using view-call, where you can mimic every field (attached-deposit / attached-gas / predecessor-account-id / signer-account-id), and it should work even with storage mutation (just that mutations should not be stored). Allowing cross-contract calls in view mode would be good, but it is more challenging. It would require the client to be tracking shards from all touched contracts (plus, it requires extra logic to mimic block delays between cross-contract calls, probably).

frol · 2022-10-24T11:19:57Z

@mfornet Do you think this NEP is ready to be reviewed by the whole Tooling Working Group and potentially getting into the voting stage next?

ori-near · 2022-12-08T22:41:37Z

As @frol mentioned above, we would like to schedule a working group meeting to review this NEP. Before we proceed, we would like to get closer to a consensus on this NEP.

@volovyks @DavidM-D @ailisp Could you please fully read this NEP and comment in the thread if you are leaning with approving or rejecting it? Please make sure to include your rationale and any feedback that you have for the author. Thank you.

ailisp · 2023-01-04T09:18:37Z

As a working group member, I lean towards approving this NEP based on the rationale and security concerns. Based on my knowledge on protocol and contract tooling, proposed solution is reasonably good addition to the tooling with trivial security implications to the protocol. The drawbacks is acceptable. The unsolved issue is also not a big problem.

volovyks · 2023-01-05T14:57:25Z

As a working group member, I lean towards approving this proposal based on existing discussions in this PR, Zullip and rationale from @austinabell in the NEP itself. The breaking change created by this change is extremely unlikly to affect real contracts. At the same time I agree that it will improve DevEx.

Other proposals, like predecessor_acount_id should be discussed separately, since they can affect more users.

DavidM-D · 2023-01-09T12:24:05Z

I also approve this proposal. It seems sensible and well thought through.

ori-near · 2023-01-16T20:21:17Z

As the moderator, I would like to thank the author @austinabell for submitting this NEP, and for the @near/wg-tooling working group members for your review. Based on the voting indications above, it seems like this proposal is close to a decision. We are therefore scheduling the first Tools Working group meeting this week, where this NEP can enter the final voting stage. Anyone can discuss the technical details by adding your comments to this discussion. And join the call to learn about the final decision and how to get more involved.

Meeting Info
📅 Date: Thursday, Jan 19, 8 AM UTC
✏️ Register

ori-near · 2023-01-20T16:36:32Z

Thank you to those who attended the first Tools Working Group meeting yesterday! The working group members reviewed the NEP and reached the following consensus:

Status: Approved

Meeting Recording:
https://youtu.be/poVmblmc3L4

@austinabell Thank you for authoring this NEP!

Next Steps:

@austinabell OR @near/nep-moderators To embed the discussed benefits & Concerns section into the NEP document
@near/nep-moderators To approve and merge the NEP
~~@near/nep-moderators To post on the NEAR Gigs Board a call for champions who are interested in implementing this NEP in the tooling~~ @austinabell To create a PR in nearcore to implement this NEP: fix!(vm-logic): Remove error from attached deposit for view calls nearcore#8433
@near/nearcore-codeowners To merge nearcore implementation fix!(vm-logic): Remove error from attached deposit for view calls nearcore#8433

frol · 2023-01-24T16:20:09Z

@ailisp @volovyks May I ask any of you to add the "Decision Context" section with Benefits and Concerns into this NEP? Here is an example where I added it to NEP #351

austinabell · 2023-01-24T16:50:00Z

I updated the branch for near/nearcore#7936 (comment) but just can't push to the PR branch as it's a nearcore repo, and I don't have access. Not sure if anything else is blocking that coming in now that this seems "accepted"

…ressed in the review and mentioned in the Decision Context section

frol · 2023-01-27T11:41:15Z

@austinabell Thanks for working on the NEP and PR to nearcore!

Congrats everyone with one more NEP being merged! 🎉

) 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.

Proposal to allow attached_deposit in view contexts

4954f25

austinabell requested a review from a team as a code owner October 19, 2022 20:42

update nep #

68451f4

frol added WG-protocol Protocol Standards Work Group should be accountable S-draft/needs-moderator-review A NEP in the DRAFT stage that needs a moderator review. labels Oct 21, 2022

frol reviewed Oct 21, 2022

View reviewed changes

frol added S-voting/needs-wg-voting-indication A NEP in the VOTING stage that needs the working group voting indication. and removed S-draft/needs-moderator-review A NEP in the DRAFT stage that needs a moderator review. labels Oct 21, 2022

frol requested review from a team and mfornet October 21, 2022 18:28

frol reviewed Oct 21, 2022

View reviewed changes

neps/nep-0418.md Outdated Show resolved Hide resolved

Update neps/nep-0418.md

f46fc5c

Co-authored-by: Vlad Frolov <frolvlad@gmail.com>

change spec to not change based on protocol version

e5844ae

austinabell mentioned this pull request Oct 26, 2022

fix!(vm-logic): Remove error from attached deposit for view calls near/nearcore#7936

Closed

ori-near added WG-tools Tools Working Group should be accountable. and removed WG-protocol Protocol Standards Work Group should be accountable labels Nov 4, 2022

frol added the A-NEP A NEAR Enhancement Proposal (NEP). label Dec 20, 2022

ori-near added S-voting/final-comment-period A NEP in the final REVIEW stage. Last chance to bring up concerns before the voting call. and removed S-voting/needs-wg-voting-indication A NEP in the VOTING stage that needs the working group voting indication. labels Jan 16, 2023

ori-near added S-approved A NEP that was approved by a working group. and removed S-voting/final-comment-period A NEP in the final REVIEW stage. Last chance to bring up concerns before the voting call. labels Jan 20, 2023

austinabell mentioned this pull request Jan 24, 2023

fix!(vm-logic): Remove error from attached deposit for view calls near/nearcore#8433

Merged

frol added 4 commits January 27, 2023 12:30

chore: Added Decision Context to the NEP-418

5c71fad

chore: Updated the metadata of NEP-418

8ec5d39

chore: Removed 'Unresolved Issues' section from NEP-418 as it was add…

336bce5

…ressed in the review and mentioned in the Decision Context section

chore: Fixed code indentation (replaced tabs with spaces) in NEP-418

ae75c95

frol approved these changes Jan 27, 2023

View reviewed changes

frol merged commit 1ec32ec into master Jan 27, 2023

frol deleted the austin/view_attached_deposit branch January 27, 2023 11:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal to allow attached_deposit in view contexts #418

Proposal to allow attached_deposit in view contexts #418

austinabell commented Oct 19, 2022 •

edited

Loading

render bot commented Oct 19, 2022

mfornet commented Oct 20, 2022

frol Oct 21, 2022

austinabell Oct 21, 2022 •

edited

Loading

mfornet Oct 22, 2022

austinabell Oct 26, 2022

frol commented Oct 21, 2022 •

edited by ori-near

Loading

frol commented Oct 21, 2022

mfornet commented Oct 22, 2022

frol commented Oct 24, 2022 •

edited by ori-near

Loading

ori-near commented Dec 8, 2022 •

edited

Loading

ailisp commented Jan 4, 2023

volovyks commented Jan 5, 2023

DavidM-D commented Jan 9, 2023

ori-near commented Jan 16, 2023

ori-near commented Jan 20, 2023 •

edited by frol

Loading

frol commented Jan 24, 2023

austinabell commented Jan 24, 2023

frol commented Jan 27, 2023

Proposal to allow attached_deposit in view contexts #418

Proposal to allow attached_deposit in view contexts #418

Conversation

austinabell commented Oct 19, 2022 • edited Loading

render bot commented Oct 19, 2022

mfornet commented Oct 20, 2022

frol Oct 21, 2022

Choose a reason for hiding this comment

austinabell Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

mfornet Oct 22, 2022

Choose a reason for hiding this comment

austinabell Oct 26, 2022

Choose a reason for hiding this comment

frol commented Oct 21, 2022 • edited by ori-near Loading

frol commented Oct 21, 2022

mfornet commented Oct 22, 2022

frol commented Oct 24, 2022 • edited by ori-near Loading

ori-near commented Dec 8, 2022 • edited Loading

ailisp commented Jan 4, 2023

volovyks commented Jan 5, 2023

DavidM-D commented Jan 9, 2023

ori-near commented Jan 16, 2023

ori-near commented Jan 20, 2023 • edited by frol Loading

frol commented Jan 24, 2023

austinabell commented Jan 24, 2023

frol commented Jan 27, 2023

austinabell commented Oct 19, 2022 •

edited

Loading

austinabell Oct 21, 2022 •

edited

Loading

frol commented Oct 21, 2022 •

edited by ori-near

Loading

frol commented Oct 24, 2022 •

edited by ori-near

Loading

ori-near commented Dec 8, 2022 •

edited

Loading

ori-near commented Jan 20, 2023 •

edited by frol

Loading