-
Notifications
You must be signed in to change notification settings - Fork 373
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
feat: support metadata for genesis txs #2941
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2941 +/- ##
=======================================
Coverage 63.32% 63.32%
=======================================
Files 548 548
Lines 78511 78574 +63
=======================================
+ Hits 49719 49760 +41
- Misses 25438 25459 +21
- Partials 3354 3355 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Amazing! I've made some additional modifications to |
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 personally like this solution; it's simple and elegant :)
Tested and approved 👏 🚀
The implementation seems to be heading in a very good direction. Please check my comments regarding naming and the need to avoid creating unnecessary layers of indirection. |
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.
preapproving, but check comments, thx.
I've tried a few options to change this abstraction, that you've left comments on: // GnoGenesis defines the gno genesis API,
// adopted by differing genesis state implementations
type GnoGenesis interface {
// GenesisBalances returns the genesis balances associated
// with the Gno genesis state
GenesisBalances() []Balance
// GenesisTxs returns the genesis transactions associated
// with the Gno genesis state
GenesisTxs() []GenesisTx
}
type GenesisTx interface {
// Tx returns the standard TM2 transaction
Tx() std.Tx
// Metadata returns the metadata tied
// to the tx, if any
Metadata() *GnoTxMetadata
} In the end, this is definitely, and by far, the most simple way to make this PR change, without introducing complications, and I detail why below. TL;DR:
I agree with @moul's sentiment that the gnoland application (in Attempt 1: Keeping
|
Just note that when you mention "portal loop," it also includes "gnodev," a tool that isn't "officially official" but is likely to become so. This pattern of local development and staging, where we want to replay in that manner, will probably become common in the Gno ecosystem. Did you try making: type TxWithMetadata struct {
Tx // embedded
Metadata Metadata
} I believe this can achieve the backward compatibility you're referring to. In practice, I don't care much about backward compatibility with previous testnets; test4 will be sunset soon, and test5 can launch after this change. I have a feeling that one of the constraints is your desire to make this compatible with the generic tm2 tx-archive tool: https://github.com/gnolang/tx-archive. However, the goal of creating a custom type GnoGenesis is to differentiate it from the basic tm2 Genesis. We also have GnoAccount, which extends tm2std.Account, and we will likely introduce other similar types. We are not concerned about backward compatibility with deprecated testnets. Instead, we prioritize a straightforward genesis system that makes sense for Gno and does not modify the tm2 source code. Ideally a single system, both for gnodev and for chains. Additionally, I'm considering how accurate "TxMetadata" is. Should we instead back up the block metadata and link each transaction to its block in the backup? All the missing TxMetadata is available in the block, right? It might make sense to ensure our export and restore tools use a single format, specifically the Gno format, which reflects what Gno does on top of tm2. |
This is another moment where Amino is waiting behind the corner with a baseball bat :) type MyStr struct {
A string `json:"field_a"`
B string `json:"field_b"`
}
type myStr2 struct {
MyStr
C string `json:"field_c"`
}
func main() {
s := myStr2{
MyStr: MyStr{
A: "hello",
B: "goodbye",
},
C: "salut",
}
jsonMarshal, err := json.Marshal(s)
if err != nil {
panic(err)
}
aminoMarshal, err := amino.MarshalJSON(s)
if err != nil {
panic(err)
}
fmt.Println(string(jsonMarshal))
fmt.Println(string(aminoMarshal))
} The results you'll get are: {"field_a":"hello","field_b":"goodbye","field_c":"salut"} // Go JSON
{"MyStr":{"field_a":"hello","field_b":"goodbye"},"field_c":"salut"} // Amino JSON Not to mention that the field needs to be exported in Amino (which wouldn't be a problem for us).
I've actually not considered how tools will need to adapt to this change, but scoped it only to the monorepo changes that need to be made -- and they're everything apart from minimal if we don't have an abstraction like we do now 🙁 I agree backwards compatibility is not an issue, it's definitely not a dealbreaker for me at this point. The biggest worry I have is bundling super use-case specific logic (portal loop, gnodev) with our core chain / future multichain logic. I don't think having |
Blocks are essential for maintaining a chain history. A TxMetadata or tx-export format isn't necessary to run a chain; it's merely an intermediary format we chose because it made sense (as an intermediary format). It allows us to create genesis from an export. Since this format is not used by the chain in real-time and does not synchronize the chain, we should design it in a way that we believe is most effective for us and for tools. In my opinion, a tx-export format or TxWithMetadata should either include the missing metadata or link to the block metadata in some way. Let's strive to find the best compromise regarding size and other factors. When we establish this new format, we should not view it as an exception for Regarding struct embedding and Amino, that's unfortunate. Currently, I prefer not to create an interface but to define TxMetadata as the format we want for the tx-exports tool as well, an official new type defined in the |
@@ -409,10 +409,17 @@ func generateGenesisFile(genesisFile string, pk crypto.PubKey, c *startCfg) erro | |||
|
|||
genesisTxs = append(pkgsTxs, genesisTxs...) | |||
|
|||
metadataTxs := make([]gnoland.TxWithMetadata, 0, len(genesisTxs)) |
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.
Can we avoid this and patch the genesisTxs file accordingly?
@@ -19,5 +21,37 @@ func parseTxs(txFile string) ([]std.Tx, error) { | |||
} | |||
defer file.Close() | |||
|
|||
return std.ParseTxs(context.Background(), file) |
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.
You can create a similar helper in the gnoland package.
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.
LGTM
Description
This PR introduces metadata support for genesis transactions (such as timestamps), in the form of a new Gno genesis state that's easily generate-able.
Shoutout to @clockworkgr for sanity checking the idea, and providing insights that ultimately led to this PR materializing.
BREAKING CHANGE
The
GnoGenesisState
is now modified, and all functionality that references it (ex.gnogenesis
,tx-archive
) will need to adapt.What we wanted to accomplish
The Portal Loop does not save "time" information upon restarting (from block 0). This means that any transaction that resulted in a Realm / Package calling
time.Now()
will get differing results when these same transactions are "replayed" as part of the loop (in the genesis state).We wanted to somehow preserve this timestamp information when the transactions (from a previous loop), are executed as part of the genesis building process.
For example:
time.Now()
, which returns time T, and saves it somewhere (Realm state)It is worth noting that this functionality is something we want in
gnodev
, so simple helpers on the Realms to update the timestamps wouldn't work.What this PR does
I've tried to follow a couple of principles when working on this PR:
I'm not a huge fan of execution hooks, so the solution proposed in this PR doesn't rely on any hook mechanism. Not going with the hook approach also significantly decreases the complexity and preserves readability.
The base of this solution is enabling execution context modification, with minimal / no API changes.
Having functionality like this, we can tailor the context during critical segments such as genesis generation, and we're not just limited to timestamps (which is the primary use-case).
We also introduce a new type of
AppState
, calledMetadataGenesisState
, where metadata is associated with the transactions. We hide the actualAppState
implementation behind an interface, so existing tools and flows don't break, and work as normal.What this PR doesn't do
There is more work to be done if this PR is merged:
tx-archive
for supporting exporting txs with metadata. Should be straightforward to dognoland genesis
commands to support the newMetadataGenesisState
. It is also straightforward, but definitely a bit of workRelated PRs and issues:
cc @moul @thehowl @jeronimoalbi @ilgooz
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description