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

r3v4s · 2024-03-18T08:52:04Z

BREAKING CHANGE: changes native banker behavior

It closes #1786

realm that uses banker should only send coins owned by itself, not by others

Contributors' checklist...

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests
Added new benchmarks to generated graphs, if any. More info here.

codecov · 2024-03-18T08:55:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.49%. Comparing base (7596f42) to head (9dae510).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1787      +/-   ##
==========================================
+ Coverage   44.79%   47.49%   +2.69%     
==========================================
  Files         459      388      -71     
  Lines       67642    61310    -6332     
==========================================
- Hits        30300    29117    -1183     
+ Misses      34808    29755    -5053     
+ Partials     2534     2438      -96

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ajnavarro · 2024-03-19T10:18:00Z

@r3v4s could you rebase please? Thanks!

leohhhn

Thank you for noticing and fixing this bug! 🙏

r3v4s · 2024-03-20T01:55:54Z

@r3v4s could you rebase please? Thanks!

did rebase, nothing conflicts at all

leohhhn · 2024-03-20T10:26:28Z

Hey, it seems there is an example test failing in the CI. Please fix it and then we can merge 🙏

r3v4s · 2024-03-21T02:11:58Z

Hey, it seems there is an example test failing in the CI. Please fix it and then we can merge 🙏

Fixed :D

thehowl · 2024-03-21T16:52:58Z

note, I think that it would be better if after we created a Banker, Send was allowed to send coins from the address of the realm that created the banker rather than the one calling Send.

In any case, this fixes current behaviour and we can implement the above in another PR. Merging.

piux2

we should not modify this testing case.

piux2 · 2024-04-04T07:12:21Z

examples/gno.land/r/demo/banktest/banktest.gno

@@ -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()


why do we modify this testing case?

piux2 · 2024-04-04T07:13:06Z

examples/gno.land/r/demo/banktest/z_0_filetest.gno

@@ -21,7 +21,6 @@ func main() {
 	println("main before:", mainbal) // plus OrigSend equals 300.

 	// simulate a Deposit call.
-	std.TestSetOrigPkgAddr(banktestAddr)


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

piux2 · 2024-04-04T07:14:01Z

examples/gno.land/r/demo/banktest/z_1_filetest.gno


 	// simulate a Deposit call.
-	std.TestSetOrigPkgAddr(banktestAddr)


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

piux2 · 2024-04-04T07:59:15Z

examples/gno.land/r/gnoland/faucet/faucet.gno

@@ -43,7 +43,7 @@ func Transfer(to std.Address, send int64) string {
 	gTotalTransfers++

 	banker := std.GetBanker(std.BankerTypeRealmSend)
-	pkgaddr := std.GetOrigPkgAddr()


Why do we make this change?

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

piux2 · 2024-04-04T10:24:18Z

gno.land/cmd/gnoland/testdata/issue-1786.txtar

+# 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"


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

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

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)

piux2 · 2024-04-04T11:28:33Z

gnovm/stdlibs/std/banker.go

 	if bt == btOrigSend || bt == btRealmSend {
-		if from != ctx.OrigPkgAddr {
+		if from != pkgAddr {


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

gno/gnovm/stdlibs/std/banker.go

Line 97 in 6c5b4cf

if from != osb.pkgAddr {

gno/gnovm/stdlibs/std/banker.go

Line 144 in 6c5b4cf

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.

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

piux2 · 2024-04-09T17:30:04Z

@r3v4s can you check out the comments and questions here? Thanks!

r3v4s · 2024-04-11T05:47:01Z

@r3v4s can you check out the comments and questions here? Thanks!

Hello piux2, just checked all of your comments and fixed few things that need to be fix.
Please check new commits.

BTW since 'this' PR is merged, I think I have to create another pr to merge new changes. WDYT?

thehowl · 2024-04-11T16:05:54Z

@r3v4s

BTW since 'this' PR is merged, I think I have to create another pr to merge new changes. WDYT?

Yes please

thehowl · 2024-04-11T16:13:19Z

note, I think that it would be better if after we created a Banker, Send was allowed to send coins from the address of the realm that created the banker rather than the one calling Send.

In any case, this fixes current behaviour and we can implement the above in another PR. Merging.

@r3v4s please, in the new PR, make this change as well.

I suggest to do this you move the check into the Gno code of the banker (banker.gno). When creating the banker struct, it should contain the address of the realm that created it.

Thanks!

piux2 · 2024-04-11T16:35:34Z

@r3v4s can you check out the comments and questions here? Thanks!

Hello piux2, just checked all of your comments and fixed few things that need to be fix. Please check new commits.

BTW since 'this' PR is merged, I think I have to create another pr to merge new changes. WDYT?

Yes, let's create new pr to merge the new changes. Thanks!

r3v4s · 2024-04-12T01:55:31Z

note, I think that it would be better if after we created a Banker, Send was allowed to send coins from the address of the realm that created the banker rather than the one calling Send.
In any case, this fixes current behaviour and we can implement the above in another PR. Merging.

@r3v4s please, in the new PR, make this change as well.

I suggest to do this you move the check into the Gno code of the banker (banker.gno). When creating the banker struct, it should contain the address of the realm that created it.

Thanks!

Like this idea ❤️. Furthermore if address of the realm that created banker gets include in banker struct I don't see any reason to keep from parameter for SendCoins(). Any opinion about removing it?

UPDATE: Ignore it, seems to be vm keeper heavily relies on from parameter

…er (#1921) related pr #1787 There was bit of extra conversion in previous pr after merged. 1) revert test cases 2) allow `Send` from realm that created banker rather the one calling  <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> --------- Co-authored-by: Morgan <[email protected]>

…er (gnolang#1921) related pr gnolang#1787 There was bit of extra conversion in previous pr after merged. 1) revert test cases 2) allow `Send` from realm that created banker rather the one calling  <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> --------- Co-authored-by: Morgan <[email protected]>

r3v4s requested review from jaekwon, thehowl, moul and a team as code owners March 18, 2024 08:52

github-actions bot assigned r3v4s Mar 18, 2024

github-actions bot added 🧾 package/realm Tag used for new Realms or Packages. 📦 🤖 gnovm Issues or PRs gnovm related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Mar 18, 2024

ajnavarro approved these changes Mar 19, 2024

View reviewed changes

leohhhn approved these changes Mar 19, 2024

View reviewed changes

r3v4s force-pushed the fix/banker-permission branch from 8885a31 to c9d0a9b Compare March 20, 2024 01:54

r3v4s added 3 commits March 21, 2024 11:12

fix: banker permission

1f74cc1

fix: banktest

5a27bc8

fix: faucet

fd94473

r3v4s force-pushed the fix/banker-permission branch from 51e86b5 to fd94473 Compare March 21, 2024 02:12

r3v4s added the breaking change Functionality that contains breaking changes label Mar 21, 2024

docs: update missing link

9dae510

thehowl approved these changes Mar 21, 2024

View reviewed changes

thehowl changed the title ~~fix: (realm with) banker to send only coins owned by itself~~ fix(stdlibs/std): Banker should only send coins owned by the calling realm Mar 21, 2024

thehowl merged commit 728f3f2 into gnolang:master Mar 21, 2024
192 of 193 checks passed

r3v4s deleted the fix/banker-permission branch March 22, 2024 04:58

piux2 reviewed Apr 4, 2024

View reviewed changes

gfanton mentioned this pull request Apr 4, 2024

Minutes: GitHub Review Meetings [every Thursday] gnolang/meetings#18

Open

3 tasks

r3v4s mentioned this pull request Apr 12, 2024

feat(stdlibs/std): restrict Banker methods based on caller of GetBanker #1921

Merged

7 tasks

Kouteki mentioned this pull request Apr 12, 2024

Minutes: Core Staff Weekly Syncs [every Monday] gnolang/meetings#36

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

r3v4s commented Mar 18, 2024 •

edited by ajnavarro

Loading

codecov bot commented Mar 18, 2024 •

edited

Loading

ajnavarro commented Mar 19, 2024

leohhhn left a comment

r3v4s commented Mar 20, 2024

leohhhn commented Mar 20, 2024

r3v4s commented Mar 21, 2024

thehowl commented Mar 21, 2024

piux2 left a comment

piux2 Apr 4, 2024

r3v4s Apr 11, 2024

piux2 Apr 4, 2024 •

edited

Loading

r3v4s Apr 11, 2024

piux2 Apr 4, 2024 •

edited

Loading

r3v4s Apr 11, 2024

piux2 Apr 4, 2024 •

edited

Loading

r3v4s Apr 11, 2024

piux2 Apr 4, 2024

r3v4s Apr 11, 2024 •

edited

Loading

piux2 Apr 4, 2024 •

edited

Loading

r3v4s Apr 11, 2024 •

edited

Loading

piux2 commented Apr 9, 2024

r3v4s commented Apr 11, 2024

thehowl commented Apr 11, 2024

thehowl commented Apr 11, 2024

piux2 commented Apr 11, 2024 •

edited

Loading

r3v4s commented Apr 12, 2024 •

edited

Loading


		// simulate a Deposit call.
		std.TestSetOrigPkgAddr(banktestAddr)

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

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

Conversation

r3v4s commented Mar 18, 2024 • edited by ajnavarro Loading

BREAKING CHANGE: changes native banker behavior

codecov bot commented Mar 18, 2024 • edited Loading

Codecov Report

ajnavarro commented Mar 19, 2024

leohhhn left a comment

Choose a reason for hiding this comment

r3v4s commented Mar 20, 2024

leohhhn commented Mar 20, 2024

r3v4s commented Mar 21, 2024

thehowl commented Mar 21, 2024

piux2 left a comment

Choose a reason for hiding this comment

piux2 Apr 4, 2024

Choose a reason for hiding this comment

r3v4s Apr 11, 2024

Choose a reason for hiding this comment

piux2 Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

r3v4s Apr 11, 2024

Choose a reason for hiding this comment

piux2 Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

r3v4s Apr 11, 2024

Choose a reason for hiding this comment

piux2 Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

r3v4s Apr 11, 2024

Choose a reason for hiding this comment

piux2 Apr 4, 2024

Choose a reason for hiding this comment

r3v4s Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

piux2 Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

r3v4s Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

piux2 commented Apr 9, 2024

r3v4s commented Apr 11, 2024

thehowl commented Apr 11, 2024

thehowl commented Apr 11, 2024

piux2 commented Apr 11, 2024 • edited Loading

r3v4s commented Apr 12, 2024 • edited Loading

r3v4s commented Mar 18, 2024 •

edited by ajnavarro

Loading

codecov bot commented Mar 18, 2024 •

edited

Loading

piux2 Apr 4, 2024 •

edited

Loading

piux2 Apr 4, 2024 •

edited

Loading

piux2 Apr 4, 2024 •

edited

Loading

r3v4s Apr 11, 2024 •

edited

Loading

piux2 Apr 4, 2024 •

edited

Loading

r3v4s Apr 11, 2024 •

edited

Loading

piux2 commented Apr 11, 2024 •

edited

Loading

r3v4s commented Apr 12, 2024 •

edited

Loading