From d09f797f5f37010c89c4fe58b779919eb90a642e Mon Sep 17 00:00:00 2001 From: Wojtek <103407812+wojtek-coreum@users.noreply.github.com> Date: Thu, 26 Oct 2023 13:39:02 +0200 Subject: [PATCH 1/2] Delete `nolint:dupl` annotations in test files (#686) # Description We disabled `dupl` linter for test files. # 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 - [x] Provide a concise and meaningful description - [x] Review the code yourself first, before making the PR. - [x] Annotate your PR in places that require explanation. - [x] Think and try to split the PR to smaller PR if it is big. --- integration-tests/modules/assetft_test.go | 8 -------- x/asset/ft/keeper/keeper_test.go | 2 -- x/asset/ft/types/msgs_test.go | 2 -- x/asset/ft/types/token_test.go | 2 -- x/asset/nft/types/msgs_test.go | 5 ----- 5 files changed, 19 deletions(-) diff --git a/integration-tests/modules/assetft_test.go b/integration-tests/modules/assetft_test.go index 8a7a982e7..c9708c8b5 100644 --- a/integration-tests/modules/assetft_test.go +++ b/integration-tests/modules/assetft_test.go @@ -758,8 +758,6 @@ func TestAssetFTBurn(t *testing.T) { } // TestAssetFTBurnRate tests burn rate functionality of fungible tokens. -// -//nolint:dupl func TestAssetFTBurnRate(t *testing.T) { t.Parallel() @@ -905,8 +903,6 @@ func TestAssetFTBurnRate(t *testing.T) { } // TestAssetFTSendCommissionRate tests send commission rate functionality of fungible tokens. -// -//nolint:dupl func TestAssetFTSendCommissionRate(t *testing.T) { t.Parallel() @@ -1823,8 +1819,6 @@ func TestSendCoreTokenWithRestrictedToken(t *testing.T) { } // TestNotEnoughBalanceForBurnRate checks tx will fail if there is not enough balance to cover burn rate. -// -//nolint:dupl // we expect code duplication in tests func TestNotEnoughBalanceForBurnRate(t *testing.T) { t.Parallel() @@ -1902,8 +1896,6 @@ func TestNotEnoughBalanceForBurnRate(t *testing.T) { } // TestNotEnoughBalanceForCommissionRate checks tx will fail if there is not enough balance to cover commission rate. -// -//nolint:dupl // we expect code duplication in tests func TestNotEnoughBalanceForCommissionRate(t *testing.T) { t.Parallel() diff --git a/x/asset/ft/keeper/keeper_test.go b/x/asset/ft/keeper/keeper_test.go index 0f62a02c0..c7c169e70 100644 --- a/x/asset/ft/keeper/keeper_test.go +++ b/x/asset/ft/keeper/keeper_test.go @@ -511,7 +511,6 @@ func TestKeeper_Burn(t *testing.T) { requireT.ErrorIs(err, cosmoserrors.ErrInsufficientFunds) } -//nolint:dupl // We don't care func TestKeeper_BurnRate_BankSend(t *testing.T) { requireT := require.New(t) @@ -759,7 +758,6 @@ func TestKeeper_BurnRate_BankMultiSend(t *testing.T) { } } -//nolint:dupl // We don't care func TestKeeper_SendCommissionRate_BankSend(t *testing.T) { requireT := require.New(t) diff --git a/x/asset/ft/types/msgs_test.go b/x/asset/ft/types/msgs_test.go index c8a367f6e..584ed6949 100644 --- a/x/asset/ft/types/msgs_test.go +++ b/x/asset/ft/types/msgs_test.go @@ -305,7 +305,6 @@ func TestMsgUnfreeze_ValidateBasic(t *testing.T) { } } -//nolint:dupl // tests and mint tests are identical, but merging them is not beneficial func TestMsgMint_ValidateBasic(t *testing.T) { type M = types.MsgMint @@ -352,7 +351,6 @@ func TestMsgMint_ValidateBasic(t *testing.T) { } } -//nolint:dupl // tests and mint tests are identical, but merging them is not beneficial func TestMsgBurn_ValidateBasic(t *testing.T) { type M = types.MsgBurn diff --git a/x/asset/ft/types/token_test.go b/x/asset/ft/types/token_test.go index bc2e5a319..465268bf0 100644 --- a/x/asset/ft/types/token_test.go +++ b/x/asset/ft/types/token_test.go @@ -280,7 +280,6 @@ func TestValidateFeatures(t *testing.T) { } } -//nolint:dupl // We don't care func TestValidateBurnRate(t *testing.T) { testCases := []struct { rate string @@ -363,7 +362,6 @@ func TestValidateBurnRate(t *testing.T) { } } -//nolint:dupl // We don't care func TestValidateSendCommissionRate(t *testing.T) { testCases := []struct { rate string diff --git a/x/asset/nft/types/msgs_test.go b/x/asset/nft/types/msgs_test.go index 78bdb8b3f..061e9630a 100644 --- a/x/asset/nft/types/msgs_test.go +++ b/x/asset/nft/types/msgs_test.go @@ -324,7 +324,6 @@ func TestMsgMint_ValidateBasic(t *testing.T) { } } -//nolint:dupl // test case duplicates are ok func TestMsgBurn_ValidateBasic(t *testing.T) { validMessage := types.MsgBurn{ Sender: "devcore172rc5sz2uclpsy3vvx3y79ah5dk450z5ruq2r5", @@ -386,7 +385,6 @@ func TestMsgBurn_ValidateBasic(t *testing.T) { } } -//nolint:dupl // test case duplicates are ok func TestMsgFreeze_ValidateBasic(t *testing.T) { validMessage := types.MsgFreeze{ Sender: "devcore172rc5sz2uclpsy3vvx3y79ah5dk450z5ruq2r5", @@ -448,7 +446,6 @@ func TestMsgFreeze_ValidateBasic(t *testing.T) { } } -//nolint:dupl // test case duplicates are ok func TestMsgUnfreeze_ValidateBasic(t *testing.T) { validMessage := types.MsgUnfreeze{ Sender: "devcore172rc5sz2uclpsy3vvx3y79ah5dk450z5ruq2r5", @@ -510,7 +507,6 @@ func TestMsgUnfreeze_ValidateBasic(t *testing.T) { } } -//nolint:dupl // test case duplicates are ok func TestMsgAddToWhitelist_ValidateBasic(t *testing.T) { validMessage := types.MsgAddToWhitelist{ Sender: "devcore172rc5sz2uclpsy3vvx3y79ah5dk450z5ruq2r5", @@ -582,7 +578,6 @@ func TestMsgAddToWhitelist_ValidateBasic(t *testing.T) { } } -//nolint:dupl // test case duplicates are ok func TestMsgRemoveFromWhitelist_ValidateBasic(t *testing.T) { validMessage := types.MsgRemoveFromWhitelist{ Sender: "devcore172rc5sz2uclpsy3vvx3y79ah5dk450z5ruq2r5", From 6b189cea242065401c8c802ac435d4e2e376402c Mon Sep 17 00:00:00 2001 From: Wojtek <103407812+wojtek-coreum@users.noreply.github.com> Date: Thu, 26 Oct 2023 14:14:38 +0200 Subject: [PATCH 2/2] Estimate overhead required by authz execution (#687) # Description # 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 - [x] Provide a concise and meaningful description - [x] Review the code yourself first, before making the PR. - [x] Annotate your PR in places that require explanation. - [x] Think and try to split the PR to smaller PR if it is big. --- .../modules/gas_estimation_test.go | 83 +++++++++++++++++++ x/deterministicgas/config.go | 7 +- x/deterministicgas/spec/README.md | 2 +- 3 files changed, 85 insertions(+), 7 deletions(-) diff --git a/integration-tests/modules/gas_estimation_test.go b/integration-tests/modules/gas_estimation_test.go index 04d08863e..73198c3e7 100644 --- a/integration-tests/modules/gas_estimation_test.go +++ b/integration-tests/modules/gas_estimation_test.go @@ -5,10 +5,13 @@ package modules import ( "fmt" "testing" + "time" sdkmath "cosmossdk.io/math" sdk "github.com/cosmos/cosmos-sdk/types" + authztypes "github.com/cosmos/cosmos-sdk/x/authz" banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" + "github.com/samber/lo" "github.com/stretchr/testify/require" integrationtests "github.com/CoreumFoundation/coreum/v3/integration-tests" @@ -114,3 +117,83 @@ func TestBankSendEstimation(t *testing.T) { fmt.Printf("%d\t%d\n", n, txRes.GasUsed) } } + +// TestAuthzEstimation it estimates gas overhead required by authz message execution. +// It executes regular message first. Then the same message is executed using authz. By subtracting those values +// we know what the overhead of authz is. +// To get correct results, both authz and bank send must be temporarily configured as non-deterministic messages, +// to get real results. +func TestAuthzEstimation(t *testing.T) { + t.Parallel() + + ctx, chain := integrationtests.NewCoreumTestingContext(t) + + requireT := require.New(t) + + granter := chain.GenAccount() + grantee := chain.GenAccount() + recipient1 := chain.GenAccount() + recipient2 := chain.GenAccount() + + chain.Faucet.FundAccounts(ctx, t, + integration.FundedAccount{ + Address: granter, + Amount: chain.NewCoin(sdk.NewInt(50000000)), + }, + integration.FundedAccount{ + Address: grantee, + Amount: chain.NewCoin(sdk.NewInt(50000000)), + }, + ) + + // grant the authorization + grantMsg, err := authztypes.NewMsgGrant( + granter, + grantee, + authztypes.NewGenericAuthorization(sdk.MsgTypeURL(&banktypes.MsgSend{})), + lo.ToPtr(time.Now().Add(time.Minute)), + ) + require.NoError(t, err) + + _, err = client.BroadcastTx( + ctx, + chain.ClientContext.WithFromAddress(granter), + chain.TxFactory().WithGas(chain.GasLimitByMsgs(grantMsg)), + grantMsg, + ) + requireT.NoError(err) + + // execute regular message + amountToSend := sdkmath.NewInt(2_000) + txf := chain.TxFactory().WithSimulateAndExecute(true) + resRegular, err := client.BroadcastTx( + ctx, + chain.ClientContext.WithFromAddress(granter), + txf, + &banktypes.MsgSend{ + FromAddress: granter.String(), + ToAddress: recipient1.String(), + Amount: sdk.NewCoins(chain.NewCoin(amountToSend)), + }, + ) + requireT.NoError(err) + + // execute authz message + execMsg := authztypes.NewMsgExec(grantee, []sdk.Msg{ + &banktypes.MsgSend{ + FromAddress: granter.String(), + ToAddress: recipient2.String(), + Amount: sdk.NewCoins(chain.NewCoin(amountToSend)), + }, + }) + + resAuthZ, err := client.BroadcastTx( + ctx, + chain.ClientContext.WithFromAddress(grantee), + txf, + &execMsg, + ) + requireT.NoError(err) + + fmt.Printf("Authz gas overhead: %d\n", resAuthZ.GasUsed-resRegular.GasUsed) +} diff --git a/x/deterministicgas/config.go b/x/deterministicgas/config.go index 1ec46fc28..072d757cf 100644 --- a/x/deterministicgas/config.go +++ b/x/deterministicgas/config.go @@ -35,7 +35,7 @@ import ( const ( BankSendPerCoinGas = 50000 BankMultiSendPerOperationsGas = 35000 - AuthzExecOverhead = 2000 + AuthzExecOverhead = 1500 ) type ( @@ -95,11 +95,6 @@ func DefaultConfig() Config { MsgToMsgURL(&assetnfttypes.MsgRemoveFromClassWhitelist{}): constantGasFunc(3500), // authz - // FIXME (v47-deterministic): We need a procedure to estimate the overhead of the authz. Proposal: - // 1. Estimate normal message - // 2. Estimate the same message executed using authz - // 3. Subtract one from the other - // We should have an integration test doing this. MsgToMsgURL(&authz.MsgExec{}): cfg.authzMsgExecGasFunc(AuthzExecOverhead), MsgToMsgURL(&authz.MsgGrant{}): constantGasFunc(28000), MsgToMsgURL(&authz.MsgRevoke{}): constantGasFunc(8000), diff --git a/x/deterministicgas/spec/README.md b/x/deterministicgas/spec/README.md index 656dcd616..c0adfac0a 100644 --- a/x/deterministicgas/spec/README.md +++ b/x/deterministicgas/spec/README.md @@ -143,7 +143,7 @@ Real examples of special case tests could be found [here](https://github.com/Cor `DeterministicGasForMsg = authzMsgExecOverhead + Sum(DeterministicGas(ChildMsg))` -`authzMsgExecOverhead` is currently equal to `2000`. +`authzMsgExecOverhead` is currently equal to `1500`. ### Nondeterministic messages