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.

Sign up for GitHub

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

Jump to bottom

feat: ExecAsPkg #644

Closed
wants to merge 3 commits into from
Closed

feat: ExecAsPkg #644

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
41d04a5
chore: add std.ExecAsPkg
moul Sep 13, 2022
46d1415
feat: exec-as-pkg
albttx Mar 24, 2023
d59c320
replace OpCall with OpPreCall
tbruyelle Mar 24, 2023
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
9 changes: 9 additions & 0 deletions pkgs/gnolang/machine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,9 @@ const (
OpValueDecl Op = 0x90 // var/const ...
OpTypeDecl Op = 0x91 // type ...

/* Context operators */
OpSetContext Op = 0xA0

/* Loop (sticky) operators (>= 0xD0) */
OpSticky Op = 0xD0 // not a real op.
OpBody Op = 0xD1 // if/block/switch/select.
Expand Down Expand Up @@ -938,6 +941,9 @@ const (
OpCPUValueDecl = 1
OpCPUTypeDecl = 1

/* Context operators */
OpCPUContext = 1

/* Loop (sticky) operators (>= 0xD0) */
OpCPUSticky = 1
OpCPUBody = 1
Expand Down Expand Up @@ -1275,6 +1281,9 @@ func (m *Machine) Run() {
case OpReturnCallDefers:
m.incrCPU(OpCPUReturnCallDefers)
m.doOpReturnCallDefers()
case OpSetContext:
m.incrCPU(OpCPUContext)
m.doOpSetContext()
default:
panic(fmt.Sprintf("unexpected opcode %s", op.String()))
}
Expand Down
5 changes: 5 additions & 0 deletions pkgs/gnolang/op_call.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,3 +407,8 @@ func (m *Machine) doOpPanic2() {
m.PushOp(OpReturnCallDefers) // XXX rename, not return?
}
}

func (m *Machine) doOpSetContext() {
cv := m.PopValue().V.(ContextValue)
m.Context = cv.Context
}
13 changes: 13 additions & 0 deletions pkgs/gnolang/values.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ func (*PackageValue) assertValue() {}
func (*NativeValue) assertValue() {}
func (*Block) assertValue() {}
func (RefValue) assertValue() {}
func (ContextValue) assertValue() {}

const (
nilStr = "nil"
Expand All @@ -62,6 +63,7 @@ var (
_ Value = &NativeValue{}
_ Value = &Block{}
_ Value = RefValue{}
_ Value = ContextValue{}
)

// ----------------------------------------
Expand Down Expand Up @@ -2184,6 +2186,17 @@ func (tv *TypedValue) GetSlice2(alloc *Allocator, low, high, max int) TypedValue
}
}

// ----------------------------------------
// ContextValue

type ContextValue struct {
Context interface{}
}

func (ctx ContextValue) String() string {
return fmt.Sprintf("%v", ctx.Context)
}

// ----------------------------------------
// Block
//
Expand Down
42 changes: 42 additions & 0 deletions stdlibs/stdlibs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,48 @@ func InjectPackage(store gno.Store, pn *gno.PackageNode) {
m.PushValue(res0)
},
)
pn.DefineNative("ExecAsPkg",
// TODO: rename UnsafeExecAsPkg?
Copy link
Member

Choose a reason for hiding this comment

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

I'm still uncertain, so I'd appreciate additional input. However, it appears that we have three primary options.

  • std.ExecAsPkg
  • std.UnsafeExecAsPkg
  • unsafe.ExecAsPkg

I believe it is essential to reassess our understanding of safety and risk when drafting contracts. After all, isn't the default assumption that everything is potentially unsafe?

Copy link
Member Author

@albttx albttx Mar 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I havn't removed your comments.

But i think we should keep std.ExecAsPkg, it's a "normal feature" for a smart-contract engine to be able to do actions as the smart-contract addr

The use of the word Unsafe could scare contract writers, and make them think as a bad practice.

Copy link
Contributor

@piux2 piux2 Mar 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I havn't removed your comments.

But i think we should keep std.ExecAsPkg, it's a "normal feature" for a smart-contract engine to be able to do actions as the smart-contract addr

The use of the word Unsafe could scare contract writers, and make them think as a bad practice.

@albttx

Gnolang support pass function as parameters and interface callback. It does exactly what current std.ExecAsPkg's implementation tries to do, and is more flexible and safer.

Not sure if std.ExecAsPkg implementation does type checking on the function parameters.

pn.DefineNative("ExecAsPkg",
// TODO: rename UnsafeExecAsPkg?
gno.Flds( // params
"fn", gno.FuncT(nil, nil),
),

gno/stdlibs/stdlibs.go

Line 281 in d59c320

"fn", gno.FuncT(nil, nil),

On the other hand, Gnolang, as in go, supports the passing functions as parameters does type checking of the passed functions' parameter types

To solve #634, as you mentioned, you can modify GRC20 with interface callback or pass a callback function as a parameter to allow the airdrop contract to alter the state of your token contract. In addition, you can add a static allowed list or even try to verify interface types for approval.

That is more idiomatic in go programming, safe and auditable.

Copy link
Contributor

@piux2 piux2 Mar 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

gno/tests/files/zrealm_std6.gno

Line 21 in d59c320

// g157y5v3k529jyzhjjz4fn49tzzhf4gess6v39xg

  • Testing case is not robust. It exposes a safety issue for the contract to use std.ExecAsPkg. All three lines should have output the same address without knowing call path context has been changed.

  • It sideloads the behavior of std.GetOrigCaller() and makes contracts hard to audit. The safety and security of smart contracts have very much to do with readability and auditability.

  • More importantly, It makes checking std.GetOrigCaller() lose its purpose when we call std.ExecAsPkg(anyFunction).

Here is why:

std.GetOrigCaller() suppose to get the first caller's address of the entire call path. Usually, it is end users who initiate a function call on a smart contract. In Ethereum, that is an external owned account (EOA). In gno, we do not separate it since the main package can also be the OrigCaller.

No matter where std.GetOrigCaller() is called in the call path; it should always return the same address.

However, std.ExecAsPkg changes the context of the call path in the middle. It makes std.GetOrigCaller() returns the address of the contract that std.ExecAsPkg is called in.

gno/stdlibs/stdlibs.go

Line 308 in d59c320

m.PushOp(gno.OpSetContext) 

// Push an alternate context with OrigCaller=OrigPkgAddr, this will
// affect the function call above
m.PushOp(gno.OpSetContext)
ctx := m.Context.(ExecContext)
ctx.OrigCaller = ctx.OrigPkgAddr

It makes checking std.GetOrigCaller() lose its purpose, when we call std.ExecAsPkg( any function ) in the same contract.

gno.Flds( // params
"fn", gno.FuncT(nil, nil),
),
gno.Flds( // results
),
func(m *gno.Machine) {
// TODO: limit stack size: 1? 32?.
// TODO: limit usage in goroutines?
// TODO: build a new sub-context instead of patching the root one.
// to support multiple concurrent contexts and advanced flows.
arg0 := m.LastBlock().GetParams1().TV
fn := arg0.GetFunc()

ctx := m.Context.(ExecContext)
ctx.OrigCaller = ctx.OrigPkgAddr

backupContext := m.Context.(ExecContext)

m.PushValue(gno.TypedValue{
T: &gno.InterfaceType{},
V: gno.ContextValue{
Context: backupContext,
},
})
m.PushOp(gno.OpSetContext)

cx := gno.Call(gno.FuncT(nil, nil))
m.PushValue(gno.TypedValue{})
Copy link
Contributor

@tbruyelle tbruyelle Mar 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We are still unsure why we need to add this useless value in the values' stack.

The thing is, if we don't, the stack is truncated by one value (when PopFrameReset is invoked). In that case, the truncated value is the context value we pushed earlier, and doOpSetContext cannot work properly without it.

We'll try to figure out why this value is truncated.

Copy link
Contributor

Choose a reason for hiding this comment

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

After some investigation, in a normal context, this truncated value is the FuncValue that will be invoked. In this particular context (native call), the FuncValue comes from the native function parameter, so it's not in the values' stack.

Finally, instead of using OpCall, we should use OpPreCall that is the layer just above. This is done here
d59c320, and thankfully, we no longer need to push a placeholder value.

m.PushFrameCall(cx, fn, gno.TypedValue{})
m.PushOp(gno.OpCall)

m.PushValue(gno.TypedValue{
T: &gno.InterfaceType{},
V: gno.ContextValue{
Context: ctx,
},
})
m.PushOp(gno.OpSetContext)
},
)
pn.DefineNative("GetCallerAt",
gno.Flds( // params
"n", "int",
Expand Down
4 changes: 4 additions & 0 deletions stdlibs/stdshim/stdshim.gno
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,3 +78,7 @@ func DecodeBech32(addr Address) (prefix string, bytes [20]byte, ok bool) {
func DerivePkgAddr(pkgPath string) (addr Address) {
panic(shimWarn)
}

func ExecAsPkg(fn func()) {
panic(shimWarn)
}
2 changes: 1 addition & 1 deletion tests/file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func TestMachine(store gno.Store, stdout io.Writer, pkgPath string) *gno.Machine
func testMachineCustom(store gno.Store, pkgPath string, stdout io.Writer, maxAlloc int64, send std.Coins) *gno.Machine {
// FIXME: create a better package to manage this, with custom constructors
pkgAddr := gno.DerivePkgAddr(pkgPath) // the addr of the pkgPath called.
caller := gno.DerivePkgAddr(pkgPath) // NOTE: for the purpose of testing, the caller is generally the "main" package, same as pkgAddr.
caller := gno.DerivePkgAddr("caller") // NOTE: for the purpose of testing, the caller is generally the "main" package, same as pkgAddr.
pkgCoins := std.MustParseCoins("200000000ugnot").Add(send) // >= send.
banker := newTestBanker(pkgAddr.Bech32(), pkgCoins)
ctx := stdlibs.ExecContext{
Expand Down
20 changes: 20 additions & 0 deletions tests/files/zrealm_std6.gno
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// PKGPATH: gno.land/r/std_test
package std_test

import (
"fmt"
"std"
)

func main() {
println(std.GetOrigCaller())
std.ExecAsPkg(func() {
println(std.GetOrigCaller())
})
println(std.GetOrigCaller())
}

// Output:
Copy link
Member

Choose a reason for hiding this comment

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

We need more tests for this feature: multiple levels; reused func pointers; touching a variable outside of the scope; panic; probably more.

// g1fahslvxakc058u0wa98ytfc9fd6trhe5qfh9ld
// g157y5v3k529jyzhjjz4fn49tzzhf4gess6v39xg
// g1fahslvxakc058u0wa98ytfc9fd6trhe5qfh9ld