refactor(golang/cosmos): Simplify and DRY out isPrimaryUpgradeName

[gibson042]

Ref #10088 (comment)

Description

Converts upgradeNamesOfThisVersion from []string to map[string]bool to avoid repeating upgrade names when defining which are primary, simplifying isPrimaryUpgradeName to a simple map lookup.

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

n/a

Testing Considerations

n/a

Upgrade Considerations

This also simplifies the release process, removing the need for it to include changing isPrimaryUpgradeName.

[cloudflare-workers-and-pages]

Deploying agoric-sdk with    Cloudflare Pages

Latest commit:	8b9e834
Status:	✅  Deploy successful!
Preview URL:	https://18441c3c.agoric-sdk.pages.dev
Branch Preview URL:	https://gibson-2024-09-refactor-ispr.agoric-sdk.pages.dev

View logs

[mhofman]

I'd like to see some of the logic partially restored

[mhofman]

While no longer in use right now, this helper was extremely useful when we had to generate different core proposal configs depending on the upgrade name (different price feed depending on the network). Let keep it around.

Maybe restore the check that name is a valid upgrade name in isPrimaryUpgradeName ?

[gibson042]

isPrimaryUpgradeName already includes that check... if the provided name is not a valid upgrade name, then isPrimary, found := upgradeNamesOfThisVersion[name] will set both isPrimary and found to false, the latter resulting in a panic two lines down. And any non-panicky validity check could be trivially performed against upgradeNamesOfThisVersion in exactly the same way.

[mhofman]

What I'm asking here is to keep validUpgradeName around as it was useful for a switch case that we needed when conditionally generating core proposals. I figured calling it from isPrimaryUpgradeName was an easy way to keep it in use to satisfy golint checks.

[gibson042]

Can you point to that switch case? I think I'd also like it better without validUpgradeName.

[mhofman]

Need to do some git blaming to track the refactor that introduced it, but the concept was first introduced here:

agoric-sdk/golang/cosmos/app/upgrade.go

Line 37 in ab43404

if !upgradeNamesOfThisVersion[expectedUpgradeName] {

[gibson042]

Yeah, for such a release I'd probably just generalize upgradeNamesOfThisVersion from map[string]bool to map[string][]string (with each value containing the correct oracle addresses). And if we want that kind of generalization to be obvious, I can update this PR from map[string]bool to map[string]*upgradeMetaData (where the basic upgradeMetaData is just struct{ isPrimary bool }).

[mhofman]

what is the concern with keeping part of the customization related to upgrade-name imperative instead of through configuration? In JS a switch case is perfectly acceptable to do this. The problem is that in golang we don't have types to help us check a typo wasn't made.

[gibson042]

It requires the upgrade names to be retyped and muddies their source of truth, creating another opportunity for error in release branch initialization (which in fact I did miss in #10088). And it's not actually imperative anyway, since that link demonstrates that there was in fact a static mapping from upgrade name to data that was serialized into a JSON template. Basically, it's unnecessary complexity.

[mhofman]

In that particular case there was only a config change indeed. I would like a flexible option that allows conditionally including some core proposals depending on the upgrade name. What do you suggest is the most robust approach?

[gibson042]

I suggest generalizing upgradeNamesOfThisVersion:

// upgradeDataOfThisVersion documents the current upgrade names and whether
// each is "primary" (used to trigger store migrations, which can only run
// once), sometimes with other data as well (such as chain-specific core
// proposal names).
// An actual release should have exactly one primary upgrade name and any number
// of non-primary upgrade names (each of the latter being associated with a
// release candidate iteration after the first RC and used only for
// pre-production chains), but for testing purposes, the master branch has
// multiple primary upgrade names.
var upgradeDataOfThisVersion = map[string]*upgradeData{
	"UNRELEASED_BASIC":           &upgradeData{true, "foo"},
	"UNRELEASED_A3P_INTEGRATION": &upgradeData{true, "foo"},
	"UNRELEASED_main":            &upgradeData{true, "real"},
	"UNRELEASED_devnet":          &upgradeData{true, "phony"},
	"UNRELEASED_REAPPLY":         &upgradeData{false, ""},
}

type upgradeData struct {
	isPrimary bool
	// Extra fields for this particular version go here.
	coreProposalName string
}