-
Notifications
You must be signed in to change notification settings - Fork 413
feat(gnovm): initial inter-realm spec implementation #2958
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
(The "review team" label was added by mistake. This PR is opened by a core dev.) |
Are there any specific behavioral patterns in Gno that would be introduced by this PR? If yes, let me know, so I can document them and share with the community. |
there is, I'll provide a short doc for you so that you can expand, as soon as possible. |
look'n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
Mainly cosmetic suggestions.
// If the object is unreal but already owned, | ||
// it means the object is bound to this realm | ||
// and must be attached to it. | ||
// otherwise it zero |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// otherwise it zero |
pkgIDFromPkgPathCache[path] = pkgID | ||
} | ||
return *pkgID | ||
} | ||
|
||
// Returns the ObjectID of the PackageValue associated with path. | ||
// ObjectIDFromPkgPath the [ObjectID] of the [PackageValue] associated with path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// ObjectIDFromPkgPath the [ObjectID] of the [PackageValue] associated with path. | |
// ObjectIDFromPkgPath returns the ObjectID of the PackageValue associated with path. |
Using [
for emphasis can be confusing. I had to verify that you were not dealing with []ObjecID
or []PackageValue
.
func ObjectIDFromPkgPath(path string) ObjectID { | ||
pkgID := PkgIDFromPkgPath(path) | ||
return ObjectIDFromPkgID(pkgID) | ||
} | ||
|
||
// Returns the ObjectID of the PackageValue associated with pkgID. | ||
// ObjectIDFromPkgID the [ObjectID] of the [PackageValue] associated with pkgID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// ObjectIDFromPkgID the [ObjectID] of the [PackageValue] associated with pkgID. | |
// ObjectIDFromPkgID returns the ObjectID of the PackageValue associated with pkgID. |
|
||
created []Object // about to become real. | ||
updated []Object // real objects that were modified. | ||
deleted []Object // real objects that became deleted. | ||
escaped []Object // real objects with refcount > 1. | ||
} | ||
|
||
// Creates a blank new realm with counter 0. | ||
// NewRealm a blank new [Realm] with counter 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// NewRealm a blank new [Realm] with counter 0. | |
// NewRealm creates a blank new Realm. |
// during a transaction, these actions are recorded through [Realm.DidUpdate]. | ||
// When a realm boundary is exited, then [Realm.FinalizeRealmTransaction] is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// during a transaction, these actions are recorded through [Realm.DidUpdate]. | |
// When a realm boundary is exited, then [Realm.FinalizeRealmTransaction] is | |
// during a transaction, these actions are recorded through Realm.DidUpdate. | |
// When a realm boundary is crossed, then Realm.FinalizeRealmTransaction is |
ID PkgID | ||
// ID is a constant-sized hash for the realm's package path. | ||
ID PkgID | ||
// The realm package path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// The realm package path. | |
// Path is the realm package path. |
// The "Time" of a realm; a variable increased for each new object, in order | ||
// to assign it a unique [ObjectID]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// The "Time" of a realm; a variable increased for each new object, in order | |
// to assign it a unique [ObjectID]. | |
// Time is a counter to provide a unique ObjectID to each new object. |
Co-authored-by: Marc Vertes <mvertes@free.fr>
Co-authored-by: Marc Vertes <mvertes@free.fr>
…nto dev/maxwell/cross-realm
@@ -112,15 +114,16 @@ type Object interface { | |||
SetIsDeleted(bool, uint64) | |||
GetIsNewReal() bool | |||
SetIsNewReal(bool) | |||
GetBoundRealm() PkgID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The word "bound" is not used in the inter-realm spec; it is "attached" to the realm.
Confusing vs BoundMethod
// is deduced from type, otherwise zero. | ||
boundRealm = getPkgId(tv.T) | ||
default: | ||
// do nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
panic, so future object types (if any) can be implemented and the panic help ensure to consider the logic here.
case *Block: | ||
// assert to pointer value | ||
if pv, ok := tv.V.(PointerValue); ok { | ||
boundRealm = getPkgId(pv.TV.T) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if tv.V.(PointerValue), and tv.GetFirstObject() is *Block,
pv.TV is the thing being pointed to,
but getPkgId(pv.TV.T) has no relation to the block obj.
attachment comes from the LHS, all this logic is coming from the RHS
@@ -46,7 +46,7 @@ func (m *Machine) doOpAddAssign() { | |||
// add rv to lv. | |||
addAssign(m.Alloc, lv.TV, rv) | |||
if lv.Base != nil { | |||
m.Realm.DidUpdate(lv.Base.(Object), nil, nil) | |||
m.Realm.DidUpdate(m.Store, lv.Base.(Object), nil, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it should be necessary to pass store in for inter-realm logic.
There's nothing in the spec that would necessitate loading anything from the store.
https://docs.google.com/document/d/1eCgGPCJ8fMhNc_vc_pbgCxP10X7jGKLC89nBUGatsD4/edit?tab=t.0
If something is in the store it is a RefValue, and from the RefValue you can tell what realm it's attached (stored) to. No need to load anything.
Probably because it's trying to do things from the RHS, and not purely from the LHS, as described here: https://github.com/gnolang/gno/pull/2958/files#r1986207268
I'm making another PR to demonstrate a simpler impl.
close as fixed by #4037 . |
address: the inter realm spec part from #2743.
Please refer to the original doc, and #2743 (comment)
And please see the comments below for details of implementation .
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description