feat(cosmos): contextual and declarative VM action field defaults #8703

michaelfig · 2023-12-31T04:50:49Z

closes: #8676

Description

Combine Cosmos SDK context and type tags with reflection to reduce the boilerplate needed to create "actions" to be sent to the SwingSet VM.

The pioneering work was done by @gibson042 in #8676, I just tried to make it more thorough and use it throughout golang/cosmos.

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

Agoric-Cosmos internals-only, should be documented in comments to give clues, but need not be published elsewhere.

Testing Considerations

Unit tests of vm.WithActionContext, otherwise reliant on existing tests for basic functionality.

Upgrade Considerations

n/a

JimLarson

Approved, but please see strong suggestions in comments.

JimLarson · 2024-01-03T03:23:26Z

golang/cosmos/vm/action.go

+		if !field.IsZero() {
+			continue
+		}


Cute, but consider an explicit continue for each case in the switch.

JimLarson · 2024-01-03T03:26:07Z

golang/cosmos/vm/action.go

+
+// WithActionContext interprets `default:"..."` tags and adds other fields from
+// the SDK context to the action.
+func WithActionContext(ctx sdk.Context, action Jsonable) Jsonable {


Consider renaming to PopulateAction(). The "with...context" triggers me to think of the various "with" methods for sdk.Context.

JimLarson · 2024-01-03T03:31:48Z

golang/cosmos/x/swingset/abci.go

+	Type        string       `json:"type" default:"BEGIN_BLOCK"`
 	BlockHeight int64        `json:"blockHeight"`
 	BlockTime   int64        `json:"blockTime"`


Here and below, consider embedding a common ActionHeader structure as in Richard's version. Also, make some reference to the understanding that certain fields will be automagically populated. Having the common header lets you put that reference in just one place.

mhofman · 2024-01-04T23:18:12Z

Removing the automerge label, was reviewing

mhofman

This is a nice consolidation. I am a little nervous about the init action change, but it's likely fine. Please consider addressing my nits before re-enabling automerge.

mhofman · 2024-01-04T23:09:10Z

golang/cosmos/app/app.go

-	var blockTime int64 = 0
-	if bootstrap || app.upgradePlan != nil {
-		blockTime = ctx.BlockTime().Unix()
-	}


I am uncomfortable with providing a blockTime and blockHeight for the init action sent during simple restart scenarios since there is so much non determinism in this case.

Can you explain more about "so much non-determinism"? Are we trying to paper over a Cosmos bug, or is there some subtlety that the cosmic-swingset side of things needs to understand?

The AG_COSMOS_INIT handler is already very careful in its assumptions, and does not look at blockTime/blockHeight unless isBootstrap or upgradePlan. I don't think it's beneficial to override the Golang propagation of these values when we may need them someday in the handler.

If it's really Golang that should change, I would prefer changing the construction of the ctx sdk.Context passed into app.SwingSetKeeper.BlockingSend, not in special-casing the ActionHeader population. That to me falls into the "papering over a Cosmos bug" category, and would necessitate a bunch of justifying commentary so that the reader would be left with an (uncomfortable) understanding of the bug.

There is no bug. It's just how we use the init "blocking send", and that a node can be started either after consensus stop, or from per validator action. I was simply concerned about including consensus state in actions happening outside of consensus. But you're right, there isn't really a problem as long as no consensus decision is taken from that data.

Thanks. I will add a comment describing that to the init blocking send call.

mhofman · 2024-01-04T23:25:16Z

golang/cosmos/vm/action.go

+	// Default the Type field to the default tag.
+	if len(defaultType) != 0 && len(ah.Type) == 0 {
+		ah.Type = defaultType
+	}


Is there a case where defaultType would not be set?

Should we assert that Type is never empty?

The semantics I'm aiming for is "never fail, just set defaults when they're available". It's reasonable to make Type a mandatory non-empty field in PushAction and BlockingSend, though. I'll do that.

mhofman · 2024-01-04T23:26:53Z

golang/cosmos/vm/action.go

+		// Populate the embedded *ActionHeader struct.
+		if fieldType.Anonymous && fieldType.Type == actionHeaderType {
+			ahp := field.Addr().Interface().(*ActionHeader)
+			setActionHeaderDefaults(ctx, defaultTag, ahp)


It feels weird to use the default tag an an embedded header like this. I know it might be more verbose, but I think a defaultType tag would be more appropriate.

Okay, I can do that.

JimLarson · 2024-01-05T02:37:55Z

Note that the non-action bridge messages will change format when we switch to JSON-RPC as part of the split brain effort.

feat(cosmos): contextual and declarative VM action field defaults

michaelfig added agd Agoric (Golang) Daemon agoric-cosmos labels Dec 31, 2023

michaelfig requested review from gibson042 and JimLarson December 31, 2023 04:50

michaelfig self-assigned this Dec 31, 2023

michaelfig force-pushed the mfig-vm-action-defaults branch from ff87a1c to 6511df8 Compare December 31, 2023 05:32

michaelfig marked this pull request as ready for review December 31, 2023 16:20

michaelfig force-pushed the mfig-vm-action-defaults branch 3 times, most recently from 53c04f3 to 405e2d5 Compare December 31, 2023 19:28

michaelfig changed the title ~~Declarative VM action defaults~~ feat(cosmos): contextual and declarative VM action field default values Dec 31, 2023

michaelfig changed the title ~~feat(cosmos): contextual and declarative VM action field default values~~ feat(cosmos): contextual and declarative VM action field defaults Dec 31, 2023

JimLarson approved these changes Jan 3, 2024

View reviewed changes

mhofman self-requested a review January 4, 2024 21:56

michaelfig force-pushed the mfig-vm-action-defaults branch from 405e2d5 to 040fda4 Compare January 4, 2024 23:10

michaelfig added the automerge:no-update (expert!) Automatically merge without updates label Jan 4, 2024

mhofman removed the automerge:no-update (expert!) Automatically merge without updates label Jan 4, 2024

mhofman reviewed Jan 4, 2024

View reviewed changes

michaelfig force-pushed the mfig-vm-action-defaults branch from 74c27e8 to 33e6c1a Compare January 5, 2024 03:35

michaelfig added the automerge:no-update (expert!) Automatically merge without updates label Jan 5, 2024

michaelfig added 2 commits January 5, 2024 18:27

feat(cosmos): impose defaults when sending VM actions

a710d68

refactor(cosmos): rely on contextual defaults for VM actions

021fd0f

michaelfig force-pushed the mfig-vm-action-defaults branch from 33e6c1a to 021fd0f Compare January 6, 2024 00:28

mergify bot merged commit 75ef233 into master Jan 6, 2024
67 checks passed

mergify bot deleted the mfig-vm-action-defaults branch January 6, 2024 00:58

mhofman pushed a commit that referenced this pull request Jan 30, 2024

Merge pull request #8703 from Agoric/mfig-vm-action-defaults

e0eea09

feat(cosmos): contextual and declarative VM action field defaults

mhofman mentioned this pull request Jan 30, 2024

Cherry-pick sequential core proposals and create-dapp #8842

Merged

gibson042 mentioned this pull request Feb 1, 2024

docs(golang): Improve the description of PopulateAction #8855

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cosmos): contextual and declarative VM action field defaults #8703

feat(cosmos): contextual and declarative VM action field defaults #8703

michaelfig commented Dec 31, 2023 •

edited

Loading

JimLarson left a comment

JimLarson Jan 3, 2024

JimLarson Jan 3, 2024

JimLarson Jan 3, 2024

mhofman commented Jan 4, 2024 •

edited

Loading

mhofman left a comment

mhofman Jan 4, 2024

michaelfig Jan 5, 2024

mhofman Jan 5, 2024

michaelfig Jan 5, 2024

mhofman Jan 4, 2024

michaelfig Jan 5, 2024

mhofman Jan 4, 2024

michaelfig Jan 5, 2024

JimLarson commented Jan 5, 2024

feat(cosmos): contextual and declarative VM action field defaults #8703

feat(cosmos): contextual and declarative VM action field defaults #8703

Conversation

michaelfig commented Dec 31, 2023 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

JimLarson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman commented Jan 4, 2024 • edited Loading

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimLarson commented Jan 5, 2024

michaelfig commented Dec 31, 2023 •

edited

Loading

mhofman commented Jan 4, 2024 •

edited

Loading