Skip to content
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

Add asset ft issuance operation simulation. #748

Merged
merged 11 commits into from
Dec 20, 2023
Merged

Conversation

dzmitryhil
Copy link
Contributor

@dzmitryhil dzmitryhil commented Dec 13, 2023

Description

Add asset ft issuance operation simulation.

Reviewers checklist:

  • Try to write more meaningful comments with clear actions to be taken.
  • Nit-picking should be unblocking. Focus on core issues.

Authors checklist

  • Provide a concise and meaningful description
  • Review the code yourself first, before making the PR.
  • Annotate your PR in places that require explanation.
  • Think and try to split the PR to smaller PR if it is big.

This change is Reviewable

@dzmitryhil dzmitryhil requested a review from a team as a code owner December 13, 2023 12:53
@dzmitryhil dzmitryhil requested review from miladz68, ysv and wojtek-coreum and removed request for a team December 13, 2023 12:53
Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)


app/simulation_test.go line 96 at r1 (raw file):

// fauxMerkleModeOpt returns a BaseApp option to use a dbStoreAdapter instead of
// an IAVLStore for faster simulation speed.
func fauxMerkleModeOpt(bapp *baseapp.BaseApp) {

minor: Maybe just define this function in the line where it is used


x/asset/ft/simulation/operations.go line 71 at r1 (raw file):

}

func (op *OperationFactory) simulateMsgIssue() simtypes.Operation {

There are no arguments here so you may remove the external function


x/asset/ft/simulation/operations.go line 125 at r1 (raw file):

}

func (op *OperationFactory) sendMsg(

it seems to be a common function to all the message types, no?


x/deterministicgas/types/gas.go line 18 at r1 (raw file):

const (
	fuseGasMultiplier    = 1000

I know you explained this, but I don't remember. Why is it needed?

Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


app/simulation_test.go line 96 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

minor: Maybe just define this function in the line where it is used

We will reuse it later since we will have different test types.


x/asset/ft/simulation/operations.go line 71 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

There are no arguments here so you may remove the external function

Didn't get that comment


x/asset/ft/simulation/operations.go line 125 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

it seems to be a common function to all the message types, no?

For now we don't use it anywther else. So I would keep it here until we have different use case.


x/deterministicgas/types/gas.go line 18 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

I know you explained this, but I don't remember. Why is it needed?

The simulation fails since the 10 multiplier is not enough for the withdrawal of the validator commission. We have a task to fix it either by updating the simulation or by updating the withdrawal of the validator commission.

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, and @ysv)


x/asset/ft/simulation/operations.go line 71 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Didn't get that comment

func simulateMsgIssue(
		r *rand.Rand, app *baseapp.BaseApp, ctx sdk.Context,
		accs []simtypes.Account, chainID string,
	) (simtypes.OperationMsg, []simtypes.FutureOperation, error) {
		senderAcc, _ := simtypes.RandomAcc(r, accs)
		msg, skip := op.randomIssueMsg(ctx, r, senderAcc.Address)
		if skip {
			return simtypes.NoOpMsg(types.ModuleName, TypeMsgIssue, "skip issue"), nil, nil
		}

		err := op.sendMsg(ctx, r, chainID, []cryptotypes.PrivKey{senderAcc.PrivKey}, app, senderAcc.Address, msg)
		if err != nil {
			return simtypes.NoOpMsg(types.ModuleName, msg.Type(), "invalid issuance"), nil, err
		}

		return simtypes.NewOperationMsg(msg, false, "", nil), nil, nil
	}

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)


app/app.go line 907 at r1 (raw file):

	// exclude the nft simulation since it is incompatible with the asset nft
	simModules := excludeModules(app.ModuleManager.Modules, []string{nft.ModuleName})

do they mint NFT with invalid IDs for assetNFT ?
I'm wondering how they mint them if there is no such msg in x/nft from cosmos ?


app/simulation_test.go line 30 at r1 (raw file):

//
//	`go test ./app -run TestFullAppSimulation -v -Enabled=true \
//		-Verbose=true -NumBlocks=100 -BlockSize=200 -Commit=true -Period=5`.

-BlockSize=200 is it a number of txs ?

-Commit=true - what is the meaning of this ?


x/asset/ft/simulation/operations.go line 71 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…
func simulateMsgIssue(
		r *rand.Rand, app *baseapp.BaseApp, ctx sdk.Context,
		accs []simtypes.Account, chainID string,
	) (simtypes.OperationMsg, []simtypes.FutureOperation, error) {
		senderAcc, _ := simtypes.RandomAcc(r, accs)
		msg, skip := op.randomIssueMsg(ctx, r, senderAcc.Address)
		if skip {
			return simtypes.NoOpMsg(types.ModuleName, TypeMsgIssue, "skip issue"), nil, nil
		}

		err := op.sendMsg(ctx, r, chainID, []cryptotypes.PrivKey{senderAcc.PrivKey}, app, senderAcc.Address, msg)
		if err != nil {
			return simtypes.NoOpMsg(types.ModuleName, msg.Type(), "invalid issuance"), nil, err
		}

		return simtypes.NewOperationMsg(msg, false, "", nil), nil, nil
	}

agree with Wojtek


x/asset/ft/simulation/operations.go line 114 at r1 (raw file):

		// and the fee_collector has the asset_ft_tokens with the SendCommissionRate and BurnRate
		BurnRate: sdkmath.LegacyNewDec(int64(simtypes.RandIntBetween(r, 1, 1000))).QuoInt64(10000),
		// SendCommissionRate: sdkmath.LegacyNewDec(int64(simtypes.RandIntBetween(r, 1, 1000))).QuoInt64(10000),

but I can see that you uncommented burn_rate. Can't we uncomment send commission now also ?
I think if influences the same way, isn't it ?


x/asset/ft/simulation/operations.go line 115 at r1 (raw file):

		BurnRate: sdkmath.LegacyNewDec(int64(simtypes.RandIntBetween(r, 1, 1000))).QuoInt64(10000),
		// SendCommissionRate: sdkmath.LegacyNewDec(int64(simtypes.RandIntBetween(r, 1, 1000))).QuoInt64(10000),
		URI:     simtypes.RandStringOfLength(r, types.MaxURILength),

nit: can we generate string up to types.MaxURILength but not always ?


x/asset/ft/simulation/operations.go line 129 at r1 (raw file):

	r *rand.Rand,
	chainID string,
	privkeys []cryptotypes.PrivKey,

nit: privKeys ?


x/deterministicgas/types/gas.go line 18 at r1 (raw file):

const (
	fuseGasMultiplier    = 1000

leave todo here ?
I think we shouldn't go to prod with this value

Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


app/app.go line 907 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

do they mint NFT with invalid IDs for assetNFT ?
I'm wondering how they mint them if there is no such msg in x/nft from cosmos ?

They register them VIA keeper and genesis


app/simulation_test.go line 30 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

-BlockSize=200 is it a number of txs ?

-Commit=true - what is the meaning of this ?

You can read about those flags here: https://github.com/cosmos/cosmos-sdk/blob/v0.47.6/x/simulation/client/cli/flags.go#L36


x/asset/ft/simulation/operations.go line 71 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

agree with Wojtek

Got you, updated.


x/asset/ft/simulation/operations.go line 114 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

but I can see that you uncommented burn_rate. Can't we uncomment send commission now also ?
I think if influences the same way, isn't it ?

By mistake. The simulation doesn't work with the commission.


x/asset/ft/simulation/operations.go line 115 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: can we generate string up to types.MaxURILength but not always ?

Didn't get the comment. What do you mean but not always ?


x/asset/ft/simulation/operations.go line 129 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: privKeys ?

Done

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ysv
ysv previously approved these changes Dec 15, 2023
Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


x/asset/ft/simulation/operations.go line 115 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Didn't get the comment. What do you mean but not always ?

I mean generate string of size 1 up to types.MaxURILength

But not always generate string of types.MaxURILength size

wojtek-coreum
wojtek-coreum previously approved these changes Dec 18, 2023
Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil and @miladz68)

@dzmitryhil dzmitryhil dismissed stale reviews from wojtek-coreum and ysv via 701215f December 18, 2023 09:31
Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


x/asset/ft/simulation/operations.go line 115 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I mean generate string of size 1 up to types.MaxURILength

But not always generate string of types.MaxURILength size

But that's how it should be, isn't it?

wojtek-coreum
wojtek-coreum previously approved these changes Dec 18, 2023
Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


docs/static/openapi.json line 4039 at r4 (raw file):

      }
    },
    "/cosmos/group/v1/group_info/{group_id}": {

is it added to this PR by mistake or it should be here ?

I'm ok but I think it is a mistake


x/asset/ft/simulation/operations.go line 115 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

But that's how it should be, isn't it?

// RandStringOfLength generates a random string of a particular length

It generates string of length types.MaxURILength each time

But it should be random length

simtypes.RandStringOfLength(r, simtypes.RandIntBetween(r, 1, types.MaxURILength)))

Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


docs/static/openapi.json line 4039 at r4 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

is it added to this PR by mistake or it should be here ?

I'm ok but I think it is a mistake

The PR is already there, the squash should remove the duplicate part.


x/asset/ft/simulation/operations.go line 115 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

// RandStringOfLength generates a random string of a particular length

It generates string of length types.MaxURILength each time

But it should be random length

simtypes.RandStringOfLength(r, simtypes.RandIntBetween(r, 1, types.MaxURILength)))

Updated.


x/deterministicgas/types/gas.go line 18 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

leave todo here ?
I think we shouldn't go to prod with this value

At least not for now :-)

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @miladz68)

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @miladz68)

@dzmitryhil dzmitryhil merged commit 028736f into master Dec 20, 2023
8 checks passed
@dzmitryhil dzmitryhil deleted the dzmitryhil/sim-poc branch December 20, 2023 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants