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

fix(stdlibs/std): Banker should only send coins owned by the calling realm #1787

Merged
merged 4 commits into from
Mar 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/concepts/standard-library/coin.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,6 @@ Coins in this set are then available for access by specific types of Bankers,
which can manipulate them depending on access rights.

[//]: # (TODO ADD LINK TO Effective GNO)
Read more about coins in the [Effective Gno] section.
Read more about coins in the [Effective Gno](https://docs.gno.land/concepts/effective-gno/#native-tokens) section.

The Coin(s) API can be found in under the `std` package [reference](../../reference/standard-library/std/coin.md).
The Coin(s) API can be found in under the `std` package [reference](../../reference/standard-library/std/coin.md).
2 changes: 1 addition & 1 deletion examples/gno.land/r/demo/banktest/banktest.gno
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func Deposit(returnDenom string, returnAmount int64) string {
// return if any.
if returnAmount > 0 {
banker := std.GetBanker(std.BankerTypeOrigSend)
pkgaddr := std.GetOrigPkgAddr()
pkgaddr := std.CurrentRealm().Addr()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we modify this testing case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// TODO: use std.Coins constructors, this isn't generally safe.
banker.SendCoins(pkgaddr, caller, send)
return "returned!"
Expand Down
4 changes: 2 additions & 2 deletions examples/gno.land/r/demo/banktest/z_0_filetest.gno
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
)

func main() {
banktestAddr := std.DerivePkgAddr("gno.land/r/banktest")
banktestAddr := std.DerivePkgAddr("gno.land/r/demo/banktest")

// print main balance before.
mainaddr := std.DerivePkgAddr("main")
Expand All @@ -21,7 +21,6 @@ func main() {
println("main before:", mainbal) // plus OrigSend equals 300.

// simulate a Deposit call.
std.TestSetOrigPkgAddr(banktestAddr)
Copy link
Contributor

@piux2 piux2 Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a valid testing case, why do we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std.TestIssueCoins(banktestAddr, std.Coins{{"ugnot", 100000000}})
std.TestSetOrigSend(std.Coins{{"ugnot", 100000000}}, nil)
res := banktest.Deposit("ugnot", 100000000)
Expand All @@ -45,3 +44,4 @@ func main() {
// * g17rgsdnfxzza0sdfsdma37sdwxagsz378833ca4 100000000ugnot sent, 100000000ugnot returned, at 2009-02-13 11:31pm UTC
//
// ## total deposits
// 300000000ugnot
3 changes: 1 addition & 2 deletions examples/gno.land/r/demo/banktest/z_1_filetest.gno
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,9 @@ import (
)

func main() {
banktestAddr := std.DerivePkgAddr("gno.land/r/banktest")
banktestAddr := std.DerivePkgAddr("gno.land/r/demo/banktest")

// simulate a Deposit call.
std.TestSetOrigPkgAddr(banktestAddr)
Copy link
Contributor

@piux2 piux2 Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a valid testing case, why do we modify this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std.TestIssueCoins(banktestAddr, std.Coins{{"ugnot", 100000000}})
std.TestSetOrigSend(std.Coins{{"ugnot", 100000000}}, nil)
res := banktest.Deposit("ugnot", 101000000)
Expand Down
2 changes: 1 addition & 1 deletion examples/gno.land/r/demo/wugnot/wugnot.gno
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func Withdraw(amount uint64) {
}

caller := std.PrevRealm().Addr()
pkgaddr := std.GetOrigPkgAddr()
pkgaddr := std.CurrentRealm().Addr()

callerBal, _ := wugnot.BalanceOf(caller)
if callerBal < amount {
Expand Down
6 changes: 3 additions & 3 deletions examples/gno.land/r/gnoland/faucet/faucet.gno
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func Transfer(to std.Address, send int64) string {
gTotalTransfers++

banker := std.GetBanker(std.BankerTypeRealmSend)
pkgaddr := std.GetOrigPkgAddr()
Copy link
Contributor

@piux2 piux2 Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we make this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

banker type used here is 'RealmSend', not 'OrigSend'
I think it should be CurrentRealm()

pkgaddr := std.CurrentRealm().Addr()
banker.SendCoins(pkgaddr, to, sendCoins)
return ""
}
Expand All @@ -54,7 +54,7 @@ func GetPerTransferLimit() int64 {

func Render(_ string) string {
banker := std.GetBanker(std.BankerTypeRealmSend)
balance := banker.GetCoins(std.GetOrigPkgAddr())
balance := banker.GetCoins(std.CurrentRealm().Addr())

output := gMessage
if gInPause {
Expand All @@ -65,7 +65,7 @@ func Render(_ string) string {
output += ufmt.Sprintf("Balance: %s.\n", balance.String())
output += ufmt.Sprintf("Total transfers: %s (in %d times).\n\n", gTotalTransferred.String(), gTotalTransfers)

output += "Package address: " + std.GetOrigPkgAddr().String() + "\n\n"
output += "Package address: " + std.CurrentRealm().Addr().String() + "\n\n"
output += ufmt.Sprintf("Admin: %s\n\n ", gAdminAddr.String())
output += ufmt.Sprintf("Controllers:\n\n ")

Expand Down
11 changes: 4 additions & 7 deletions examples/gno.land/r/gnoland/faucet/faucet_test.gno
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package faucet

import (
"fmt"
"std"
"testing"

Expand All @@ -12,7 +11,7 @@ import (
func TestPackage(t *testing.T) {
var (
adminaddr = std.Address("g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5")
faucetaddr = std.DerivePkgAddr("gno.land/r/faucet")
faucetaddr = std.DerivePkgAddr("gno.land/r/gnoland/faucet")
controlleraddr1 = testutils.TestAddress("controller1")
controlleraddr2 = testutils.TestAddress("controller2")
controlleraddr3 = testutils.TestAddress("controller3")
Expand All @@ -28,10 +27,8 @@ func TestPackage(t *testing.T) {
test1addr = testutils.TestAddress("test1")
)
// deposit 1000gnot to faucet contract

std.TestIssueCoins(faucetaddr, std.Coins{{"ugnot", 1000000000}})
std.TestSetOrigPkgAddr(faucetaddr)
assertBalance(t, faucetaddr, 1000000000)
assertBalance(t, faucetaddr, 1200000000)

// by default, balance is empty, and as a user I cannot call Transfer, or Admin commands.

Expand All @@ -46,15 +43,15 @@ func TestPackage(t *testing.T) {
// as an admin, add the controller to contract and deposit more 2000gnot to contract
std.TestSetOrigCaller(adminaddr)
assertNoErr(t, faucet.AdminAddController(controlleraddr1))
assertBalance(t, faucetaddr, 1000000000)
assertBalance(t, faucetaddr, 1200000000)

// now, send some tokens as controller.
std.TestSetOrigCaller(controlleraddr1)
assertNoErr(t, faucet.Transfer(test1addr, 1000000))
assertBalance(t, test1addr, 1000000)
assertNoErr(t, faucet.Transfer(test1addr, 1000000))
assertBalance(t, test1addr, 2000000)
assertBalance(t, faucetaddr, 998000000)
assertBalance(t, faucetaddr, 1198000000)

// remove controller
// as an admin, remove controller
Expand Down
10 changes: 9 additions & 1 deletion examples/gno.land/r/gnoland/faucet/z0_filetest.gno
Original file line number Diff line number Diff line change
@@ -1,9 +1,17 @@
package main

import (
"std"

"gno.land/r/gnoland/faucet"
)

// mints ugnot to current realm
func init() {
facuetaddr := std.DerivePkgAddr("gno.land/r/gnoland/faucet")
std.TestIssueCoins(facuetaddr, std.Coins{{"ugnot", 200000000}})
}

// assert render with empty path and no controllers
func main() {
println(faucet.Render(""))
Expand All @@ -16,7 +24,7 @@ func main() {
// Balance: 200000000ugnot.
// Total transfers: (in 0 times).
//
// Package address: g17rgsdnfxzza0sdfsdma37sdwxagsz378833ca4
// Package address: g1ttrq7mp4zy6dssnmgyyktnn4hcj3ys8xhju0n7
//
// Admin: g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5
//
Expand Down
10 changes: 9 additions & 1 deletion examples/gno.land/r/gnoland/faucet/z1_filetest.gno
Original file line number Diff line number Diff line change
@@ -1,9 +1,17 @@
package main

import (
"std"

"gno.land/r/gnoland/faucet"
)

// mints ugnot to current realm
func init() {
facuetaddr := std.DerivePkgAddr("gno.land/r/gnoland/faucet")
std.TestIssueCoins(facuetaddr, std.Coins{{"ugnot", 200000000}})
}

// assert render with a path and no controllers
func main() {
println(faucet.Render("path"))
Expand All @@ -16,7 +24,7 @@ func main() {
// Balance: 200000000ugnot.
// Total transfers: (in 0 times).
//
// Package address: g17rgsdnfxzza0sdfsdma37sdwxagsz378833ca4
// Package address: g1ttrq7mp4zy6dssnmgyyktnn4hcj3ys8xhju0n7
//
// Admin: g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5
//
Expand Down
8 changes: 7 additions & 1 deletion examples/gno.land/r/gnoland/faucet/z2_filetest.gno
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ import (
"gno.land/r/gnoland/faucet"
)

// mints ugnot to current realm
func init() {
facuetaddr := std.DerivePkgAddr("gno.land/r/gnoland/faucet")
std.TestIssueCoins(facuetaddr, std.Coins{{"ugnot", 200000000}})
}

// assert render with empty path and 2 controllers
func main() {
var (
Expand All @@ -33,7 +39,7 @@ func main() {
// Balance: 200000000ugnot.
// Total transfers: (in 0 times).
//
// Package address: g17rgsdnfxzza0sdfsdma37sdwxagsz378833ca4
// Package address: g1ttrq7mp4zy6dssnmgyyktnn4hcj3ys8xhju0n7
//
// Admin: g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5
//
Expand Down
8 changes: 7 additions & 1 deletion examples/gno.land/r/gnoland/faucet/z3_filetest.gno
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ import (
"gno.land/r/gnoland/faucet"
)

// mints coints to current realm
func init() {
facuetaddr := std.DerivePkgAddr("gno.land/r/gnoland/faucet")
std.TestIssueCoins(facuetaddr, std.Coins{{"ugnot", 200000000}})
}

// assert render with 2 controllers and 2 transfers
func main() {
var (
Expand Down Expand Up @@ -45,7 +51,7 @@ func main() {
// Balance: 197000000ugnot.
// Total transfers: 3000000ugnot (in 2 times).
//
// Package address: g17rgsdnfxzza0sdfsdma37sdwxagsz378833ca4
// Package address: g1ttrq7mp4zy6dssnmgyyktnn4hcj3ys8xhju0n7
//
// Admin: g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5
//
Expand Down
86 changes: 86 additions & 0 deletions gno.land/cmd/gnoland/testdata/issue-1786.txtar
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
# Test for https://github.com/gnolang/gno/issues/1786

loadpkg gno.land/r/demo/wugnot

gnoland start

# add contract
gnokey maketx addpkg -pkgdir $WORK -pkgpath gno.land/r/demo/proxywugnot -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!

# approve wugnot to `proxywugnot ≈ g1fndyg0we60rdfchyy5dwxzkfmhl5u34j932rg3`
gnokey maketx call -pkgpath gno.land/r/demo/wugnot -func Approve -args "g1fndyg0we60rdfchyy5dwxzkfmhl5u34j932rg3" -args 10000 -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!

# send 10000ugnot to `proxywugnot` to wrap it
gnokey maketx call -pkgpath gno.land/r/demo/proxywugnot --send "10000ugnot" -func ProxyWrap -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!

# check user's wugnot balance
gnokey maketx call -pkgpath gno.land/r/demo/wugnot -func BalanceOf -args "g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5" -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!
stdout '10000 uint64'

# unwrap 500 wugnot
gnokey maketx call -pkgpath gno.land/r/demo/proxywugnot -func ProxyUnwrap -args 500 -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1

# XXX without patching anything it will panic
# panic msg: insufficient coins error
# XXX with pathcing only wugnot.gnot it will panic
# panic msg: RealmSendBanker can only send from the realm package address "g1fndyg0we60rdfchyy5dwxzkfmhl5u34j932rg3", but got "g1pf6dv9fjk3rn0m4jjcne306ga4he3mzmupfjl6"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems opposite.
g1fndyg0we60rdfchyy5dwxzkfmhl5u34j932rg3 is the address of gno.land/r/demo/proxywugnot

g1pf6dv9fjk3rn0m4jjcne306ga4he3mzmupfjl6 is the address of gno.land/r/demo/wugnot

Copy link
Contributor Author

@r3v4s r3v4s Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above msg happens(panics) when calling 'proxywugnot.Unwrap()' without patching banker.go

So we expect 'proxywugnot' (g1f) to send its own coin, not from 'wugnot' (g1p)



# check user's wugnot balance
gnokey maketx call -pkgpath gno.land/r/demo/wugnot -func BalanceOf -args "g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5" -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!
stdout '9500 uint64'

-- gno.mod --
module gno.land/r/demo/proxywugnot


-- realm.gno --
package proxywugnot

import (
"std"

"gno.land/r/demo/wugnot"

"gno.land/p/demo/ufmt"
)

func ProxyWrap() {
sent := std.GetOrigSend()
ugnotSent := uint64(sent.AmountOf("ugnot"))

if ugnotSent == 0 {
return
}

// WRAP IT
wugnotAddr := std.DerivePkgAddr("gno.land/r/demo/wugnot")
banker := std.GetBanker(std.BankerTypeRealmSend)
banker.SendCoins(std.CurrentRealm().Addr(), wugnotAddr, std.Coins{{"ugnot", int64(ugnotSent)}})
wugnot.Deposit() // `proxywugnot` has ugnot

// SEND WUGNOT: PROXY_WUGNOT -> USER
wugnot.Transfer(std.GetOrigCaller(), ugnotSent)
}

func ProxyUnwrap(wugnotAmount uint64) {
if wugnotAmount == 0 {
return
}
userOldWugnot := wugnot.BalanceOf(std.GetOrigCaller())

// SEND WUGNOT: USER -> PROXY_WUGNOT
wugnot.TransferFrom(std.GetOrigCaller(), std.CurrentRealm().Addr(), wugnotAmount)

// UNWRAP IT
wugnot.Withdraw(wugnotAmount)

// SEND GNOT: PROXY_WUGNOT -> USER
banker := std.GetBanker(std.BankerTypeRealmSend)
banker.SendCoins(std.CurrentRealm().Addr(), std.GetOrigCaller(), std.Coins{{"ugnot", int64(wugnotAmount)}})
}
10 changes: 8 additions & 2 deletions gnovm/stdlibs/std/banker.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,18 @@ func X_bankerSendCoins(m *gno.Machine, bt uint8, fromS, toS string, denoms []str
amt := CompactCoins(denoms, amounts)
from, to := crypto.Bech32Address(fromS), crypto.Bech32Address(toS)

pkgAddr := ctx.OrigPkgAddr
if m.Realm != nil {
pkgPath := m.Realm.Path
pkgAddr = gno.DerivePkgAddr(pkgPath).Bech32()
}

if bt == btOrigSend || bt == btRealmSend {
if from != ctx.OrigPkgAddr {
if from != pkgAddr {
Copy link
Contributor

@piux2 piux2 Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The earlier code checks against the package address for SendCoins() in the OrigSendBanker and RealmSendBanker separately.

if from != osb.pkgAddr {

if from != rsb.pkgAddr {

The code is the same, but the state of the package address in different banker instances could be different. In the current code, it is merged into one logic, where it checks either OrigPkgAddr or m.Realm.Path for SendCoin() of both OrigSendBanker and RealmSendBanker together. However, these should be checked separately. If not, other places that require checking OriginSendBanker's OrigPkgAddr might fail when ctx.OrignPkgAddr and m.Realm.PathAddr have different values.

Copy link
Contributor Author

@r3v4s r3v4s Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking into this
(seems this logic is also related to your another comments about test cases).

// UPDATE
Thanks for pointing this out. I missed internal behavior.
Separated verification logic 9907a3c

m.Panic(typedString(
fmt.Sprintf(
"can only send from the realm package address %q, but got %q",
ctx.OrigPkgAddr, from),
pkgAddr, from),
))
return
}
Expand Down
Loading