-
Notifications
You must be signed in to change notification settings - Fork 45
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
Some opinions on the new models #1117
base: frrist/etl-pkg
Are you sure you want to change the base?
Conversation
kasteph
commented
Jan 27, 2023
- add EventsRoot to Receipt
- add Signature to Message
- embed Message type inside VMMessage since it's basically that type here
Params []byte | ||
Receipt Receipt `gorm:"embedded"` | ||
Message Message `gorm:"embedded"` | ||
Receipt Receipt `gorm:"embedded"` |
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.
While it is true that VM messages are basically messages, many of the fields such as GasLimit
, GasFee
, and GasPremium
are all 0
. Similarly, VMMessages do not contain a Signature
, nor do their receipts contain an EventRoot
(less sure on this, but a hunch). I think it may be more efficient to avoid embedding the Message
and Receipt
structures in VmMessgae
and instead take a subset of the fields that are populated. wdyt?
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.
Looking at gorm Embedded Structs has me thinking that it's better to have common non-null types between Message
and VmMessage
wrapped like:
type BaseMessage struct {
To
From
Value
Method
Params
}
which can then be respectively added as an embedded struct for both Message
and VmMessage
. This way it'll be easier to see what they do have in common and what they don't. But this is also not a ⛰️ I'm gonna die on lol.
ExitCode int64 | ||
GasUsed int64 | ||
Return []byte | ||
EventsRoot types.DbCid |
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.
typo:
types.DbCID
and not types.DbCid