Automate x/deterministicgas doc generation using go:generate

[ysv]

This change is 

[dzmitryhil]

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)

---

x/deterministicgas/spec/generate.go line 21 at r1 (raw file):

	readmeTmpl string

	specialCases = []string{

How about using type instead of string?

---

x/deterministicgas/spec/generate.go line 40 at r1 (raw file):

	cfg := deterministicgas.DefaultConfig()
	for msgType, gasFunc := range cfg.GasByMessageMap() {
		if lo.Contains(specialCases, msgType) {

Minor, but with the map it seems like a use case for a map.

[dzmitryhil]

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)

---

x/deterministicgas/spec/README.tmpl.md line 7 at r1 (raw file):

## Intro

Coreum is using a deterministic gas model for its transactions. Meaning that given a transaction type (e.g

is using -> uses ?

Code quote:

Coreum is using a deterministic gas model for its transactions. Meaning that given a transaction type (e.g
`/coreum.asset.ft.v1.MsgIssueGasPrice`) one can know how much gas will be used beforehand, and this amount is fixed if some
preconditions are met. Of course this deterministic gas does not apply to the type of transactions that have a
complicated, nondeterministic execution path (e.g `/cosmwasm.wasm.v1.MsgExecuteContract`). We provide tables with all

[dzmitryhil]

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)

[ysv]

Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, @silverspase, and @wojtek-coreum)

---

x/deterministicgas/spec/generate.go line 21 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

How about using type instead of string?

do you mean to define a separate type for this ?
smth like this

type MsgType string

and use it instead of string everywhere ?

---

x/deterministicgas/spec/generate.go line 40 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Minor, but with the map it seems like a use case for a map.

I refactored it to map but now I realised that it introduces non-determinism because map is not ordered. As result this cycle produces different md doc each time:
for msgType := range specialCases

I can of course rewrite it with map but this will introduce more hustle

So it is probably better to use slice here

---

x/deterministicgas/spec/README.tmpl.md line 7 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

is using -> uses ?

done

[wojtek-coreum]

Reviewed 2 of 5 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

---

x/deterministicgas/config_test.go line 137 at r2 (raw file):

		t.Run("deterministic: "+deterministicgas.MsgType(sdkMsg), func(t *testing.T) {
			gas, ok := cfg.GasRequiredByMessage(sdkMsg)
			fmt.Printf("%v: %v\n", deterministicgas.MsgType(sdkMsg), gas)

leftover or you really want to print this?

---

x/deterministicgas/config_test.go line 147 at r2 (raw file):

		t.Run("nondeterministic: "+deterministicgas.MsgType(sdkMsg), func(t *testing.T) {
			gas, ok := cfg.GasRequiredByMessage(sdkMsg)
			fmt.Printf("%v: %v\n", deterministicgas.MsgType(sdkMsg), gas)

ditto

---

x/deterministicgas/spec/generate.go line 21 at r2 (raw file):

	readmeTmpl string

	specialCases = []string{

Maybe it could be done by testing that map entry is not equal to the result of constantGasFunc? To avoid the need for setting special cases in two places.

---

x/deterministicgas/spec/generate.go line 69 at r2 (raw file):

	}

	generatorComment := `[//]: # (GENERATED DOC.)

Set it inside readmeTmpl

---

x/deterministicgas/spec/generate.go line 72 at r2 (raw file):

[//]: # (DO NOT EDIT MANUALLY!!!)`

	readmeBuf := new(bytes.Buffer)

write it directly to a file, without buffer in between

---

x/deterministicgas/spec/generate.go line 90 at r2 (raw file):

		GeneratorComment:              generatorComment,
		FixedGas:                      cfg.FixedGas,
		SigVerifyCost:                 1000,

1000 -> params.SigVerifyCostSecp256k1

---

x/deterministicgas/spec/generate.go line 91 at r2 (raw file):

		FixedGas:                      cfg.FixedGas,
		SigVerifyCost:                 1000,
		TxSizeCostPerByte:             10,

10 -> params.TxSizeCostPerByte

---

x/deterministicgas/spec/README.md line 71 at r2 (raw file):

| Message Type | Gas |
|--------------|-----|
| `/cosmos.authz.v1beta1.MsgExec                               ` | [special case](#special-cases) |

spaces should be after "`"

---

x/deterministicgas/spec/README.md line 139 at r2 (raw file):

| Message Type |
|--------------|
| `/cosmos.crisis.v1beta1.MsgVerifyInvariant                   ` |

spaces should be after "`"

---

x/deterministicgas/spec/README.tmpl.md line 70 at r2 (raw file):

| Message Type | Gas |
|--------------|-----|
{{ .DeterministicMessagesTable }}

I would prefer to use {{ repeat }} and construct the table here, not in go

[dzmitryhil]

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @miladz68, @silverspase, and @ysv)

---

x/deterministicgas/spec/generate.go line 21 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

do you mean to define a separate type for this ?
smth like this

type MsgType string

and use it instead of string everywhere ?

Yes

---

x/deterministicgas/spec/generate.go line 40 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I refactored it to map but now I realised that it introduces non-determinism because map is not ordered. As result this cycle produces different md doc each time:
for msgType := range specialCases

I can of course rewrite it with map but this will introduce more hustle

So it is probably better to use slice here

Agree, resolved.

[ysv]

Reviewable status: 0 of 5 files reviewed, 11 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @wojtek-coreum)

---

x/deterministicgas/config_test.go line 137 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

leftover or you really want to print this?

Done.

---

x/deterministicgas/config_test.go line 147 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

ditto

Done.

---

x/deterministicgas/spec/generate.go line 21 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Yes

I made this change but not sure if it looks better
check it pls and let me know

---

x/deterministicgas/spec/generate.go line 21 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Maybe it could be done by testing that map entry is not equal to the result of constantGasFunc? To avoid the need for setting special cases in two places.

made it with reflection
in terms of non-duplicating this vars it definitely looks better
But in terms of tricks it's overkill IMO

---

x/deterministicgas/spec/generate.go line 69 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Set it inside readmeTmpl

I can do this but then it seems like you are not supposed to edit README.tmpl.mditself which is not true

---

x/deterministicgas/spec/generate.go line 72 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

write it directly to a file, without buffer in between

Done.

---

x/deterministicgas/spec/generate.go line 90 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

1000 -> params.SigVerifyCostSecp256k1

yes but how can I fetch these params if they are stored inside auth module params. And to access them I need Auth keeper

I would also prefer to fetch these 2 values from primary source but don't see any optimal solution for this.

---

x/deterministicgas/spec/generate.go line 91 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

10 -> params.TxSizeCostPerByte

same as prev

---

x/deterministicgas/spec/README.md line 71 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

spaces should be after "`"

Done.

---

x/deterministicgas/spec/README.md line 139 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

spaces should be after "`"

Done.

---

x/deterministicgas/spec/README.tmpl.md line 70 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

I would prefer to use {{ repeat }} and construct the table here, not in go

Done.

[wojtek-coreum]

Reviewed 4 of 5 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

---

x/deterministicgas/config.go line 194 at r4 (raw file):

// "/cosmos.distribution.v1beta1.MsgFundCommunityPool"
// "/coreum.asset.ft.v1.MsgMint".
func MsgTypeURL(msg sdk.Msg) MsgType {

That's a bit weird because MsgTypeURL returns the URL of the message type, calling it MsgType. Now, in the entire file it seems that terms "message type" and "URL of the message type" are used interchangabely, which is confusing.

---

x/deterministicgas/spec/generate.go line 21 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

made it with reflection
in terms of non-duplicating this vars it definitely looks better
But in terms of tricks it's overkill IMO

Check this commit I created on top of your PR: a1b6a51

At least some more self-describing function names might be generated for reflection.

---

x/deterministicgas/spec/generate.go line 90 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

yes but how can I fetch these params if they are stored inside auth module params. And to access them I need Auth keeper

I would also prefer to fetch these 2 values from primary source but don't see any optimal solution for this.

I don't understand. Can't you import auth module where those constants are defined?

[dzmitryhil]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)

---

x/deterministicgas/spec/generate.go line 21 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I made this change but not sure if it looks better
check it pls and let me know

IMO this version looks better.

[ysv]

Reviewable status: 2 of 6 files reviewed, 4 unresolved discussions (waiting on @miladz68, @silverspase, and @wojtek-coreum)

---

x/deterministicgas/config.go line 194 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

That's a bit weird because MsgTypeURL returns the URL of the message type, calling it MsgType. Now, in the entire file it seems that terms "message type" and "URL of the message type" are used interchangabely, which is confusing.

MsgType -> MsgURL
MsgToMsgURL()

---

x/deterministicgas/spec/generate.go line 21 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Check this commit I created on top of your PR: a1b6a51

At least some more self-describing function names might be generated for reflection.

as we agreed on the call
I will keep using current version with minor change to avoid this complex regular expressions

---

x/deterministicgas/spec/generate.go line 90 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

I don't understand. Can't you import auth module where those constants are defined?

done

---

x/deterministicgas/spec/generate.go line 91 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

same as prev

done

[wojtek-coreum]

Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68, @silverspase, and @ysv)

---

x/deterministicgas/spec/generate.go line 21 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

as we agreed on the call
I will keep using current version with minor change to avoid this complex regular expressions

The current version is ok, but if you take a look at the commit I provided above and change:

func nondeterministicGasFunc() gasByMsgFunc {
	return func(msg sdk.Msg) (uint64, bool) {
		return 0, false
	}
}

to:

func nondeterministicGasFunc(msg sdk.Msg) (uint64, bool) {
	return 0, false
}

you will simplify, both the original logic and generator

[ysv]

Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @miladz68, @silverspase, and @wojtek-coreum)

---

x/deterministicgas/spec/generate.go line 21 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

The current version is ok, but if you take a look at the commit I provided above and change:

func nondeterministicGasFunc() gasByMsgFunc {
	return func(msg sdk.Msg) (uint64, bool) {
		return 0, false
	}
}

to:

func nondeterministicGasFunc(msg sdk.Msg) (uint64, bool) {
	return 0, false
}

you will simplify, both the original logic and generator

Done.

[wojtek-coreum]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68, @silverspase, and @ysv)

---

x/deterministicgas/spec/generate.go line 38 at r6 (raw file):

	for msgURL, gasFunc := range cfg.GasByMessageMap() {
		fnFullName := runtime.FuncForPC(reflect.ValueOf(gasFunc).Pointer()).Name()
		fnParts := strings.Split(fnFullName, "/")

Non-blocking: You could split by . instead of / here, leading to shorter string in the if below

[ysv]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68 and @silverspase)

---

x/deterministicgas/spec/generate.go line 38 at r6 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Non-blocking: You could split by . instead of / here, leading to shorter string in the if below

I actually prefer longer version with deterministicgas. in prefix

[dzmitryhil]

Reviewed 2 of 5 files at r3, 2 of 4 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68 and @silverspase)