From 95b8fbbc13328dc68f771e433fde6840e391fcf1 Mon Sep 17 00:00:00 2001 From: Christian Borst Date: Mon, 26 Feb 2024 17:30:20 -0500 Subject: [PATCH 1/3] Charge liquified account gas on MsgLiquify The initial MsgLiquify implementation had the NFT deployed in the EVM by the microtx module, and ignored the gas cost incurred by the EVM operations. A contract deploy is a pretty hefty op to allow for free so to prevent spam we need to charge the user. --- x/microtx/keeper/liquid_account.go | 53 ++++++++++-------------------- 1 file changed, 18 insertions(+), 35 deletions(-) diff --git a/x/microtx/keeper/liquid_account.go b/x/microtx/keeper/liquid_account.go index dea58aa2..d1a09039 100644 --- a/x/microtx/keeper/liquid_account.go +++ b/x/microtx/keeper/liquid_account.go @@ -55,7 +55,7 @@ func (k Keeper) DoLiquify( // deployLiquidInfrastructureNFTContract deploys an NFT contract for the given `account` and then transfers ownership of the // underlying NFT to the given `account` func (k Keeper) deployLiquidInfrastructureNFTContract(ctx sdk.Context, account sdk.AccAddress) (common.Address, error) { - contract, err := k.DeployContract(ctx, types.LiquidInfrastructureNFT, account.String()) + contract, err := k.DeployContract(ctx, account, types.LiquidInfrastructureNFT, account.String()) if err != nil { return common.Address{}, sdkerrors.Wrap(err, "liquid infrastructure account contract deployment failed") } @@ -68,23 +68,9 @@ func (k Keeper) deployLiquidInfrastructureNFTContract(ctx sdk.Context, account s return common.Address{}, sdkerrors.Wrapf(err, "expected contract with version %v, got %v", CurrentNFTVersion, version) } - _, err = k.transferLiquidInfrastructureNFTFromModuleToAddress(ctx, contract, account) - if err != nil { - return common.Address{}, sdkerrors.Wrapf(err, "could not transfer nft from %v to %v", types.ModuleEVMAddress.Hex(), SDKToEVMAddress(account).Hex()) - } - return contract, nil } -// transferLiquidInfrastructureNFTFromModuleToAddress calls the ERC721 "transferFrom" method on `contract`, -// passing the arguments needed to transfer the LiquidInfrastructureNFT Account token -// from the x/microtx module account to to the `newOwner` -func (k Keeper) transferLiquidInfrastructureNFTFromModuleToAddress(ctx sdk.Context, contract common.Address, newOwner sdk.AccAddress) (*evmtypes.MsgEthereumTxResponse, error) { - // ABI: transferFrom( address from, address to, uint256 tokenId) - var args = ToMethodArgs(types.ModuleEVMAddress, SDKToEVMAddress(newOwner), &AccountId) - return k.CallMethod(ctx, "transferFrom", types.LiquidInfrastructureNFT, types.ModuleEVMAddress, &contract, &big.Int{}, args...) -} - // addLiquidInfrastructureEntry Sets a new Liquid Infrastructure Account entry in the bech32 -> EVM NFT address mapping // accAddress - The account to Liquify // nftAddress - The deployed LiquidInfrastructureNFT contract address @@ -219,7 +205,6 @@ func (k Keeper) IsLiquidAccountWithValue(ctx sdk.Context, account sdk.AccAddress // returns nil, ErrNoLiquidAccount if `nftAddress` is not a record for any Liquid Infrastructure Account func (k Keeper) GetLiquidAccountByNFTAddress(ctx sdk.Context, nftAddress common.Address) (*types.LiquidInfrastructureAccount, error) { var liquidAccount *types.LiquidInfrastructureAccount = nil - var err error = nil k.IterateLiquidAccounts( ctx, @@ -236,10 +221,6 @@ func (k Keeper) GetLiquidAccountByNFTAddress(ctx sdk.Context, nftAddress common. }, ) - if err != nil { - return nil, sdkerrors.Wrap(err, "failed to find liquid account by NFT") - } - return liquidAccount, nil } @@ -275,7 +256,6 @@ func (k Keeper) GetLiquidAccountsByEVMOwner(ctx sdk.Context, ownerAddress common // GetLiquidAccountByNFTAddress fetches info about a Liquid Infrastructure Account given the address of the LiquidInfrastructureNFT in the EVM func (k Keeper) CollectLiquidAccounts(ctx sdk.Context) ([]*types.LiquidInfrastructureAccount, error) { var liquidAccounts []*types.LiquidInfrastructureAccount - var err error = nil k.IterateLiquidAccounts( ctx, @@ -289,10 +269,6 @@ func (k Keeper) CollectLiquidAccounts(ctx sdk.Context) ([]*types.LiquidInfrastru }, ) - if err != nil { - return nil, sdkerrors.Wrap(err, "unable to collect liquid accounts") - } - return liquidAccounts, nil } @@ -431,12 +407,14 @@ func ToMethodArgs(args ...interface{}) []interface{} { // DeployContract will deploy an arbitrary smart-contract. It takes the compiled contract object as // well as an arbitrary number of arguments which will be supplied to the contructor. All contracts deployed -// are deployed by the module account. +// are deployed by the deployer account. func (k Keeper) DeployContract( ctx sdk.Context, + deployer sdk.AccAddress, contract evmtypes.CompiledContract, args ...interface{}, ) (common.Address, error) { + deployerEVM := SDKToEVMAddress(deployer) // pack constructor arguments according to compiled contract's abi // method name is nil in this case, we are calling the constructor ctorArgs, err := contract.ABI.Pack("", args...) @@ -455,7 +433,7 @@ func (k Keeper) DeployContract( copy(data[len(contract.Bin):], ctorArgs) // retrieve sequence number first to derive address if not by CREATE2 - nonce, err := k.accountKeeper.GetSequence(ctx, types.ModuleAddress.Bytes()) + nonce, err := k.accountKeeper.GetSequence(ctx, deployer) if err != nil { return common.Address{}, sdkerrors.Wrapf(types.ErrContractDeployment, @@ -463,7 +441,7 @@ func (k Keeper) DeployContract( } amount := big.NewInt(0) - _, err = k.CallEVM(ctx, types.ModuleEVMAddress, nil, amount, data, true) + _, err = k.CallEVM(ctx, deployerEVM, nil, amount, data, true) if err != nil { return common.Address{}, sdkerrors.Wrapf(types.ErrContractDeployment, @@ -471,7 +449,7 @@ func (k Keeper) DeployContract( } // Derive the newly created module smart contract using the module address and nonce - return crypto.CreateAddress(types.ModuleEVMAddress, nonce), nil + return crypto.CreateAddress(deployerEVM, nonce), nil } // CallMethod is a function to interact with a contract once it is deployed. It inputs the method name on the @@ -517,12 +495,12 @@ func (k Keeper) CallEVM( return nil, err } - // As evmKeeper.ApplyMessage does not directly increment the gas meter, any transaction - // completed through the CSR module account will technically be 'free'. As such, we can - // set the gas limit to some arbitrarily high enough number such that every transaction - // from the module account will always go through. - // see: https://github.com/evmos/ethermint/blob/35850e620d2825327a175f46ec3e8c60af84208d/x/evm/keeper/state_transition.go#L466 - gasLimit := DefaultGasLimit + cosmosLimit := ctx.GasMeter().Limit() + gasUsed := ctx.GasMeter().GasConsumed() + if cosmosLimit <= gasUsed { + return nil, sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "insufficient gas") + } + gasLimit := cosmosLimit - gasUsed // Create the EVM msg msg := ethtypes.NewMessage( @@ -545,6 +523,11 @@ func (k Keeper) CallEVM( return nil, err } + // ApplyMessage does not consume gas, so we need to consume the gas used within the EVM + // This is completely different than the ethermint gas consumption, where an AnteHandler consumes all the gas + // the EVM Tx is allowed to use, and then refunds remaining gas after execution. + ctx.GasMeter().ConsumeGas(res.GasUsed, "EVM call") + if res.Failed() { return nil, sdkerrors.Wrap(evmtypes.ErrVMExecution, res.VmError) } From 41b20f29288c386a17d37a7ce3ad4c828af86b68 Mon Sep 17 00:00:00 2001 From: Christian Borst Date: Mon, 26 Feb 2024 17:32:25 -0500 Subject: [PATCH 2/3] Fix gas in LIQUID_ACCOUNTS test now that the user is charged --- integration_tests/test_runner/src/tests/liquid_accounts.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/integration_tests/test_runner/src/tests/liquid_accounts.rs b/integration_tests/test_runner/src/tests/liquid_accounts.rs index cae7c898..2423e9c5 100644 --- a/integration_tests/test_runner/src/tests/liquid_accounts.rs +++ b/integration_tests/test_runner/src/tests/liquid_accounts.rs @@ -73,7 +73,10 @@ pub async fn liquid_accounts_test( ) .await .expect("Unable to liquify account"); - info!("Got liquify account response:\n{}", liquify_res.raw_log); + info!( + "Got liquify account response:\n{}\nTx Hash: {}\nGas used: {}", + liquify_res.raw_log, liquify_res.txhash, liquify_res.gas_used + ); let (liquid_account, _eth_owner) = assert_correct_account(web3, &mut microtx_qc, to_liquify.ethermint_address).await; @@ -227,7 +230,7 @@ pub async fn liquify_account( to_liquify, Fee { amount: vec![fee], - gas_limit: 1_000_000, + gas_limit: 2_500_000, payer: None, granter: None, }, From 0745d66056c86e07a4715dae460d4f29cd618de2 Mon Sep 17 00:00:00 2001 From: Christian Borst Date: Tue, 27 Feb 2024 11:27:09 -0500 Subject: [PATCH 3/3] Ensure accounts cannot liquify multiple times MsgLiquify deploys a NFT contract with address derived from the user's EVM address and their nonce. However the nonce is only incremented on every Tx, so if a user submits multiple MsgLiquify in the same Tx there may be deployment issues and undefined behavior. --- x/microtx/keeper/msg_server.go | 4 ++++ x/microtx/types/errors.go | 13 +++++++------ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/x/microtx/keeper/msg_server.go b/x/microtx/keeper/msg_server.go index c77419e2..2b7f4f68 100644 --- a/x/microtx/keeper/msg_server.go +++ b/x/microtx/keeper/msg_server.go @@ -166,6 +166,10 @@ func (m *msgServer) Liquify(c context.Context, msg *types.MsgLiquify) (*types.Ms return nil, sdkerrors.Wrap(sdkerrors.ErrorInvalidSigner, "liquid infrastructure accounts must use ethermint keys, perhaps this is the first message the sender has sent?") } + if m.Keeper.IsLiquidAccount(ctx, sender) { + return nil, types.ErrAccountAlreadyLiquid + } + // Call the actual liquify implementation nft, err := m.Keeper.DoLiquify(ctx, sender) if err != nil { diff --git a/x/microtx/types/errors.go b/x/microtx/types/errors.go index 961e6e64..ea5f69c3 100644 --- a/x/microtx/types/errors.go +++ b/x/microtx/types/errors.go @@ -5,10 +5,11 @@ import ( ) var ( - ErrContractDeployment = sdkerrors.Register(ModuleName, 1, "contract deploy failed") - ErrContractCall = sdkerrors.Register(ModuleName, 2, "contract call failed") - ErrNoLiquidAccount = sdkerrors.Register(ModuleName, 3, "account is not a liquid infrastructure account") - ErrInvalidThresholds = sdkerrors.Register(ModuleName, 4, "invalid liquid infrastructure account thresholds") - ErrInvalidMicrotx = sdkerrors.Register(ModuleName, 5, "invalid microtx") - ErrInvalidContract = sdkerrors.Register(ModuleName, 6, "invalid contract") + ErrContractDeployment = sdkerrors.Register(ModuleName, 1, "contract deploy failed") + ErrContractCall = sdkerrors.Register(ModuleName, 2, "contract call failed") + ErrNoLiquidAccount = sdkerrors.Register(ModuleName, 3, "account is not a liquid infrastructure account") + ErrInvalidThresholds = sdkerrors.Register(ModuleName, 4, "invalid liquid infrastructure account thresholds") + ErrInvalidMicrotx = sdkerrors.Register(ModuleName, 5, "invalid microtx") + ErrInvalidContract = sdkerrors.Register(ModuleName, 6, "invalid contract") + ErrAccountAlreadyLiquid = sdkerrors.Register(ModuleName, 7, "account is already a liquid infrastructure account") )