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

[Docs] Minting coins in Render() #1523

Closed
leohhhn opened this issue Jan 11, 2024 · 7 comments · Fixed by #2134
Closed

[Docs] Minting coins in Render() #1523

leohhhn opened this issue Jan 11, 2024 · 7 comments · Fixed by #2134

Comments

@leohhhn
Copy link
Contributor

leohhhn commented Jan 11, 2024

Description

You can mint coins via the Banker in the Render function of a Realm (or package!) currently. This is an issue because the Render function is supposed to be called a lot, and by minting coins, we are changing the state (a costly operation). Render() should be read-only.

Deploy the following code under p/demo/mint:

package mint

import "std"

func Render(path string) string {
	coin := std.Coin{"mycoin", 100}

	this := std.GetOrigPkgAddr()
	me := std.GetOrigCaller()

	banker := std.GetBanker(std.BankerTypeRealmIssue)
	banker.IssueCoin(this, coin.Denom, coin.Amount)

	return banker.GetCoins(this).String()
}

The point of this issue is to discuss the distinction between functions that change the state, and functions that do not change the state at all - and their respective gas costs. As you might know, Ethereum's approach is to have normal, gas-costing functions, and view functions that just read the state of the contract.

In order to not diverge from standard Go keywords (ie adding keywords to differentiate these), would it be possible to have the first line of the function define what type of function it is, similar to how you can define a helper in Go tests with t.Helper()?

cc @moul @zivkovicmilos @MichaelFrazzy @thehowl

@deelawn
Copy link
Contributor

deelawn commented Jan 11, 2024

Maybe we can disable calling Render as transaction and force everyone to use qrender as a query. Or reroute incoming tx requests to the query execution path when we are able to identify this is a call to Render, but I just took a look at that and it looks messy.

Follow up:
I've been thinking about other use cases of render and am not convinced that Render should not be allowed to change any state. The use case that first comes to mind is the following: There is a web app that pulls data from other sources (realms) and caches it within the local contract on an "as necessary" basis. Another use case is a counter that gets incremented each time the the Render function is invoked -- or any other types of analytics type operations.

So maybe the best solution here is no solution and the take away is don't put anything in your Render function that you wouldn't want someone to call or has potential to be abused in a way that might negatively affect the realm author.

@moul
Copy link
Member

moul commented Jan 15, 2024

This issue extends beyond the current problem. The code can be executed on Render, which will cause further confusion.

var counter

func Render() string {
	counter++
	return fmt.Sprintf("%d", counter)
}

This will increment the counter before displaying, but the change will not be applied.

I believe the same applies to the banker (incrementation is done in memory but not applied on disk). If it's not the case, then we have a bug.

In my view, the issue is more about ambiguity than performance.


I suggest creating a gno lint formula that generates a warning message when Render modifies data.

I recommend checking out this realm that heavily utilizes Render modification: https://staging.gno.land/r/demo/tamagotchi.

@deelawn
Copy link
Contributor

deelawn commented Jan 30, 2024

@moul I was just taking a closer look at this. Calling Render via maketx call will in fact change the state of the counter variable in the realm you posted. Calling render via gnoweb uses query vm/qrender under the hood to obtain a vm instance with non-persistent storage.

So do we need to make changes to prevent any Render invocations from changing state or should be make it abundantly clear in the docs that there is no guarantee Render won't change the state.

@deelawn
Copy link
Contributor

deelawn commented Apr 10, 2024

Is there any action to take or can we close this?

@zivkovicmilos
Copy link
Member

Solution proposed by @moul on our test4 alignment call:

  • Possibly rename this to dry-run or something similar and / or
  • Render stays as it currently is - it's fine to modify state, we just need to add a disclaimer in the documentation

@Kouteki
Copy link
Contributor

Kouteki commented Apr 11, 2024

Moving to Triage until we choose a solution

@Kouteki Kouteki moved this from Backlog to Triage in 🧙‍♂️gno.land core team Apr 11, 2024
@Kouteki Kouteki moved this from Triage to Todo in 🧙‍♂️gno.land core team Apr 26, 2024
@Kouteki
Copy link
Contributor

Kouteki commented Apr 26, 2024

We're going with option 2: Render stays as it currently is. We just need to add a disclaimer in the documentation.

@leohhhn leohhhn moved this to Priority 1 in 👥 Team: Devrels May 9, 2024
@Kouteki Kouteki changed the title Minting coins in Render() [Docs] Minting coins in Render() May 15, 2024
@leohhhn leohhhn moved this from Priority 1 to In Progress in 👥 Team: Devrels May 16, 2024
@leohhhn leohhhn moved this from In Progress to In Review in 👥 Team: Devrels May 17, 2024
@leohhhn leohhhn moved this from Todo to In Review in 🧙‍♂️gno.land core team May 18, 2024
thehowl pushed a commit that referenced this issue May 23, 2024
<!-- please provide a detailed description of the changes made in this
pull request. -->

## Description

After discussions in #1523, we decided to add a disclaimer to `maketx
call` which will let people know that a `call` to `Render()` will appliy
state changes.

Closes: #1523 

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
@github-project-automation github-project-automation bot moved this from In Review to Done in 👥 Team: Devrels May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants