osmosis-labs / cosmos-sdk Goto Github PK

Summary

We need a mintcoins restriction function creator, for easily creating restrictions to only mint coins with the provided prefix. E.g.

// NewPrefixMintCoinsRestriction creates a mint coins restriction fn, that only allows banks to mint coins that have a denom beginning with "...."
func NewPrefixMintCoinsRestriction(prefix string) MintCoinsRestrictionFn {
...
}

Problem Definition

We're going to use this same code for gamm and token factory.

Blocked on #48

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

go.mod

replace (
	github.com/cosmos/cosmos-sdk => github.com/osmosis-labs/cosmos-sdk v0.44.4-0.20220220052304-a117f821a9b2
)

package main

import (
	"encoding/hex"
	"fmt"

	"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
	sdk "github.com/cosmos/cosmos-sdk/types"
)

func main() {
	key := secp256k1.GenPrivKey()
	fmt.Println(hex.EncodeToString(key.GetKey()))
	pub := key.PubKey()
	addr := sdk.AccAddress(pub.Address())
	fmt.Println(addr.String())
}

The address thus generated starts with cosmos.
Like this:
b4e94efd4a3993256f6b2d9f632d654fb55a3e219d00bcb8fe3ea5349a5b0fec
cosmos1llfdqrqshdf8y7x7n8etjj3nmcn66qtkpytr46

I need the correct address format of osmos：
osmoxxxxxxxxxxxxxxx

thanks

We don't need dependabot for this repo, we effectively get it everything we update the SDK branch were on

Background

For now, CLI returns tokens denom in ibc/ format. It is hard for user to recognize what token is.

Suggested Design

Suggest from @p0mvn & @sunnya97 , we should add a config field to config.toml. It's maybe a url to assetlist then using it to convert ibc format to base format

Acceptance Criteria

Goals & criteria for success
e.g.

• all existing and new tests should pass

Summary

Problem Definition

Proposal

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

Background

Right now we're using GetSupplyWithOffset and GetSupply, which is a confusing nomenclature as we almost never need the supply without offsets.

Suggested Design

We should replace them for GetSupply and GetSupplyWithoutOffsets.

This is a backwards incompatible change and may have implications for anyone external relying on those functions, so the first question is: should we do this at all?

Background

Likely just reverting 9165099

Suggested Design

TBD

Acceptance Criteria

TBD

Summary

Add a global minimum self delegation parameter, similar to the minimum commission rate.

Problem Definition

This isn't a big restriction, as limited validator slots already effectively has this requirement. However this increases the costs to spam the number of validators. The number of validators active in the last $UNBONDING_PERIOD can have unfortunate impacts on efficiency when an app-chain's module has other amplification factors on the load caused by validators.

The base SDK has some amplification factor here, (state storage, historical signing info windows), but on brief skim nothing concerning, hence why it likely wasn't thought about before.

Proposal

Add a global minimum self delegation parameter to the codebase. For Osmosis, I suggest making it something like 100 osmo, which makes attacks that concern me very infeasible.

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

Summary

Our github project tracking for cosmos#10771

We need this in next release, if SDK folks have a different vision, we will have to go ahead and merge this for use in our next release, and switchover whenever that new vision lands. (I doubt it'd land in next 3 months, and we want this as a defense in depth relatively soon, and its a blocker for superfluid)

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

Summary of Bug

See cosmos#10694

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

The main task during the Informal Audit of Before Send hook design and implementation was to detect the possible area of impact and to see if there are places that could lead to panicking in modules affected the most.

Artifacts:

• osmosis/cosmos sdk fork at commit hash: 6fcd25f
• Full Token Factory branch holding new features

Analysis Summary:

Potential problematic places:

1. Begin Blocker function

• Fees are allocated from the fee collector account to the distribution module account within the AllocateTokens:
  

  cosmos-sdk/x/distribution/keeper/allocation.go

  Lines 31 to 34 in 5c9a51c

  	err := k.bankKeeper.SendCoinsFromModuleToModule(ctx, k.feeCollectorName, types.ModuleName, feesCollectedInt)
  	if err != nil {
  	panic(err)
  	}

Concerns:

• It was assumed that fees could be collected in different denominations, which means we could have the situation that the module would panic once the allocation of the fees in token factory native denom is frozen in a bank module.

Facts:

• Osmosis TxFees module holds the implementation of the functionality that is changing the initial assumption:
• DeductFeeDecorator is customized within Osmosis TxFees module, where fees are collected in separate module accounts with DeductFees function, depending on the denomination of the fee.
• antehandler is initialized with this deductFeeDecorator.
• On the end of each epoch, all collected non-osmo fees are swapped into osmo, and sent to the FeeCollector module account.
  AfterEpochEnd hook is implemented

Conclusion:
It seems this place is not at the risk, since allocate tokens will work only with OSMO tokens - it is not possible to trigger the hook for the native osmosis token so we could not get the situation where the FeeCollector account or Distribution account could be blocked within any of the smart contracts implementing the BeforeSend hook.

2. FundCommunityPool function calls

There were two potential "panic" cases that could occur:

• Mint module, implementation of the AfterEpochHook
• Pool incentives, implementation of AfterDistributeMintedCoin hook

Concerns:
Minted coins could be of the native token type, which could cause the FundCommunityPool function to trigger the hook and return the error, where mint module and pool incentives modules would panic.

Facts:

• Mint module only mints OSMO tokens.
• In functions: distributeDeveloperRewards and DistributeMintedCoin - Community Pool will be funded with OSMO tokens.
• Pool incentives will also allocate OSMO tokens to the community pool, with the implemented hook (see diagrams)

Conclusion:
According to the listed facts - not at the risk.

Conclusion for BeforeSend hook impact on Distribution module

It seems that a new feature has landed in the existing design with no introduction of unexpected behaviour.
However, it is necessary for the distribution module to work only with OSMO tokens in these specific places. If any change regarding this will be introduced, we would get panic.

Background

We had to undo the x/upgrades PR since it was API breaking for both wasmd and osmosis. We should reopen this PR with the respective fixes for both.

Summary

A useful metric to see what the tally would be just with validator votes only counting with self-delegation, not including delegator voting power.

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

Background

v0.45 doesn't allow an array of messages as proposals. This has been implemented in v0.46

Suggested Design

Backport the gov changes that allow for multi-msg proposals.

Q: Are these proposals executed atomically? if not, we should implement that too.

Acceptance Criteria

all tests pass
new tests for multi-msg proposals are added if needed

The main task during the Informal Audit of Before Send hook design and implementation was to detect the possible area of impact and to see if there are places that could lead to panicking in modules affected the most.

Artifacts:

• osmosis/cosmos sdk fork at commit hash: 6fcd25f
• Full Token Factory branch holding new features

Analysis Summary:

Within this issue, we will share conclusions about the possible impact on Core modules. Dependencies are visually presented in the diagram we created for this audit:

Conclusions made when taking into account: differences/similarities to the bank’s blacklisted feature:
The vanilla Bank module already has a feature that is somewhat similar to the implemented freezing of Coins transfer. Accounts can be blacklisted, and transfers from/to accounts can be “frozen” in the bank module. The error messages containing info are propagated and the call stack is taking into account that sending can be failing due to this case. (Not only insufficient funds, or missing module accounts.)

Concerns:

• Errors returned from the Send functions, once triggering the hook are not handled properly.
• Implicit assumptions in modules that sending can not be frozen. Depending on the coins that needed to be sent - but were blocked.

Facts:

• All send functions that contain hook triggers are handling the errors properly.
  
• The call stack has been analyzed for each of the paths from the diagrams, as well. (Diagrams will be shared directly with Osmosis Lab team, to not overload this issue)
• During the analysis, the business logic has been taken into account.

Conclusion for BeforeSend hook impact on Cosmos SDK Core modules

For the following modules, there is no negative impact:

• Staking, Vesting/Clawback vesting: modules will not work with native tokens created with Token Factory.
• Crisis Module: ConstantFee is paid in OSMO tokens, so this should not be an issue during VerifyInvariant, where the Sending is used.
• Governance Impact: proposals are deposited in OSMO tokens.
• Auth module's DeducFee function is not used, but the Osmosis TXFees module defines the new DeductFeeDecorator - no dependency in osmosis.
• FeeGrant module is not using a function that holds the BeforeSend trigger - no dependency in osmosis.

Potential negative impact:

• Due to the larger possible impact and analysis work done, Distribution module will be explained in separate issues:
• #319 and
• #322

Background

We should make a CI process, to test if a PR is making an API breaking change that affects modules we depend on.

Suggested Design

Have a CI workflow / python or bash script that:

• git clones a given version of IBC-go and Wasmd
  • Ideally in an adjacent directory to the SDK
• Adds a replace directive to the SDK, for their go.mods
  • e.g. replace github.com/cosmos/cosmos-sdk => /absolute/path/on_filesystem/to/cosmos-sdk
  • (Will need to play around with relative path formatting etc. Can start by running the script with an absolute path to test tho)
• Run a build command on IBC-go and Wasmd. (TBD if its literally just make build)

Acceptance Criteria

CI process alerts us to if were making an API break that affects IBC or wasmd.

Main task during the Informal Audit of Full token factory BurnFrom and ForceTransfer feature was to detect the possible area of impact.

Artifacts:

• osmosis/cosmos sdk fork at commit hash: 6fcd25f
• Full Token Factory branch holding new features

Analysis Summary done for:

• all the possible module accounts that could be set as BurnFrom address, from/to address in ForceTransfer
• the impact that the introduced feature could have on the system if it is possible to change the amounts on module accounts
• confirming modules are working with a global state that holds information that the amount/balance and supply of coins has changed due to the execution of new features
• other structures (community pool and distribution module account, locks for osmosis modules… ) that hold some information about the number of coins expected to be present on other accounts

Potential problematic places

Combination of all places in core modules using send functions and results of full token factory new features execution.

Concerns:

• Implicit assumptions that account balances can be changed only with sending coins - will the coins not existing no longer in accounts be an issue?
• Error handling in the case of smaller amounts of the coins in the accounts - so that sending can not be done.
• The possibility of sending from/to module accounts is introduced with this functionality - this could lead to module accounts working with token factory created denominations and before send hooks could be triggered in critical places analyzed with Before Send hook analysis.

Facts:

• The same places are under risk as for the before send hook feature, since those places are using sending functions (transferring funds from one account to another) and those places are under risk if expecting certain amounts.
• Manipulation with module accounts is risky. The possibility of transferring token factory-created denoms to the following accounts could be an issue:
  • x/auth FeeCollector account: this account should not be able to hold non-base denom coins - possible issues in AllocateTokens
  • osmosis x/txfees NonNativeFeeCollector account: if system has this feetoken registered, those coins will be swaped into osmo, but this account should not receive external funds -AnteHandler should be the entry point.
  • Distribution and Governance module account: assumption that module account holds only base denom
  • Vesting accounts:
    • work with base denoms
    • In the case of Burn From or ForceTransfer: locked coins can not be sent from vesting, since they are locked. For the amount greater than spendable coins - we will panic and delivering transaction will be aborted.

Conclusion:
(made with not entirely knowing business requirements for the introduced features)

• Module accounts should not be used as a destination in ForceTransfer. If necessary, enable ForceTransfer from any account to TokenFactory module account, and hold those coins here if burning them is not an option. Later, coins could be sent from token factory module account to any account. in that case supply is being preserved and tokens distributed to accounts.
• The only way for listed module accounts to hold token factory denominations is to transfer them with ForceTransfer. Osmosis works with an assumption that: staking, governance proposals are deposited /fees are collected/ coins are minted in base denom.
• FeeCollector and NonNativeFeeCollectorName accounts should also be excluded from BurnFrom and ForceTransfer From options.
• We concluded that MintTo could not introduce issues - since there are no expectations that some accounts should hold the exact amount of coins (that is, it should not be greater!)

Background

We added extra logs to debug app hash bug:
#146 (review)

Some of them should be Debug logs, not Info

Acceptance Criteria

• Move this log to Debug
• Move this log to Debug

    I think you mentioned something about the proposer not needing to rerun process proposal after prepare right? Is that something we intend to add in a later PR?

Originally posted by @czarcas7ic in #382 (comment)

Summary

We should add hooks to bank module, especially on things like sends so that we can catch them in the tokenfactory module to be able to do things like pause transfers, freeze/blacklist accounts, etc.

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

Summary of Bug

The osmosisd q slashing signing-infos command requires one argument, but actually should not require any:

$ osmosisd q slashing signing-infos --chain-id=osmosis-1 --node=http://x.x.x.x:26657 
Error: accepts 1 arg(s), received 0

However, when providing a single bogus argument it works....

$ osmosisd q slashing signing-infos "-" --chain-id=osmosis-1 --node=http://x.x.x.x:26657 -o json |jq .
{
  "info": [
    {
      "address": "osmovalcons1qq99jktrfdpfdex72djgrhsq4zswhxjc4v7mlc",
      "start_height": "312096",
      "index_offset": "447343",
...

The issue appears to be at: https://github.com/osmosis-labs/cosmos-sdk/blob/v0.42.9-osmo-v2-upgrade/x/slashing/client/cli/query.go#L84

Args: cobra.ExactArgs(1),

Which differs from the Cosmos-SDK: https://github.com/cosmos/cosmos-sdk/blob/master/x/slashing/client/cli/query.go#L83

Args: cobra.NoArgs,

I'm happy to provide a PR, just let me know which branch to point at.

Version

$ osmosisd version
3.1.0

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

System information

Osmosis version: v0.45.0x-osmo-v7
OS & Version: Linux
Commit hash: 51108b6

Expected behaviour

Osmosis currently depends on this committed version of the cosmos-sdk.
Osmosis proto generation should succeed.

Actual behaviour

It does not, because of proto file that was merged with conflicts, see here:
51108b6#diff-11eba0a7e81d48edf4761a94648f1c916c18631ef389ba684bb33cbbaf250af0R201

Steps to reproduce the behaviour

Simply try to generate the Osmosis proto or start the Osmosis chain with ignite on main branch.

Backtrace

[STARPORT] cannot build app:
[STARPORT] 
[STARPORT] 	file: /home/johndoe/go/pkg/mod/github.com/osmosis-labs/[email protected]/proto/cosmos/gov/v1beta1/gov.proto: <input>:201:1: found "<" but expected [.proto element {comment|option|import|syntax|enum|service|package|message}]

When submitting logs: please submit them as text and not screenshots.

@ValarDragon and I have had some conversations about upstreaming osmosis SDK changes into the mainline SDK.

Recently, I attempted with this commit:

#58

the SDK has make a number of changes upstream, like how tests are done, that'll make this challenging. I'm going to continue on it, basically as a learning exercise.

I am going to use this issue to track things, and don't really expect that it will be fast. I think that ideally, we need to either formally fork off, or figure out what features and changes are needed to mainline so that we can start using that.

wdyt?

Background

Currently we only have test cases for Clawback Vesting Account in the keeper layer, we need extra test cases for the cli layer as well. We're also missing minor test cases as well for ConjunctPeriods and DisjunctPeriod, which is the major methods that work on the calculation for the clawback.

We also have a room for minor refactoring on simplifying ConjunctPeriods and DisjunctPeriods(#178 (comment), #178 (comment)), AlignSchedules(#178 (comment))

Summary

We need user balances in simulation to have many different denoms. We did this originally via #9. We should probably do that again. It was removed in #43 purely due to simulation breaking on the rebase.

Problem Definition

This is needed for many important functionality tests.

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

in the v3 and v4 sdk, the command make proto-gen and related commands are broken.

Working to fix now...

Background

A job failed in an unrelated PR:
https://github.com/osmosis-labs/cosmos-sdk/runs/6014355771?check_suite_focus=true

Noticed unsynchronized shared resources reDnm and reDecCoin:
https://github.com/osmosis-labs/cosmos-sdk/blob/ed9910d57ff97a54ada98d69b2bd5ad8f22cbc7c/types/coin.go#L709-L722

Acceptance Criteria

• data race is fixed
• resources are synhronized
• unit tests added

Summary of Bug

8cbb9d2 caused the bank tests to fail, cause it changed the text of the error message

--- FAIL: TestMsgSendValidation (0.00s)
    msgs_test.go:52: 
        	Error Trace:	msgs_test.go:52
        	Error:      	Error message not equal:
        	            	expected: "Invalid recipient address (empty address string is not allowed): invalid address"
        	            	actual  : "Invalid recipient address :(decoding bech32 failed: invalid bech32 string length 0): invalid address"
        	Test:       	TestMsgSendValidation
FAIL
FAIL	github.com/cosmos/cosmos-sdk/x/bank/types	1.596s

Version

8cbb9d2

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

Summary of Bug

Reported on Discord:

After updating some of nodes to 7.1.0 we started getting grpc errors from them when querying transactions
grpc: received message larger than max (4515191 vs. 4194304)
Never had that one before

sooo i had to edit grpc server and client configurations in cosmos sdk to increase the default cap of 4MB and rebuild osmosis binary

I've set it way higher at 20MB (just to be sure), didn't have any issues since then. Will report back if it happens again

This might be related to #137

Version

v0.45.0x-osmo-v7 or v7.1.0

Acceptance Criteria

• Investigate if the cap needs to be increased and to what value
• if should increase, make a PR with the change

cref: cosmos#10353

We basically want to add a state entry to fix the overhead gov tally query is causing, having to iterate all gov votings everytime the query is called.

Background

Lets get a mergify setup, that makes it easy for us to PR a commit against SDK main.

Would be great if possible to do with maintaining the Osmosis fork developer contribution info on SDK main, but I think we can figure that out secondarily.

Suggested Design

Setup mergify in the fork to PR to SDK main

Acceptance Criteria

Adding a label automatically opens a PR against SDK main

Impact on the distribution module was analyzed for the critical places (possible panics), but also for the situations where sending of more denominations in loops could be stopped because sending of certain token factory created denom is frozen - in this issue, when handling the execution of a community pool spend proposal.

Artifacts:

• osmosis/cosmos sdk fork at commit hash: 6fcd25f
• Full Token Factory branch holding new features

Analysis Summary:

Once voting on the CommunityPoolSpendProposal has passed, distribution of funds from the feepool.CommunityPool will be activated.

Now we are analyzing denominations that could be added to the Community pool:

but in the context of later distributing those funds to the recipient, specified with a proposal:

Concerns:

• If a community pool holds denominations created with a token factory - we expect the distribution to pass for those denoms which are not blocked for sending by the hook execution. For others, distribution should be skipped.
  The error could be obtained for sending the amount from the distribution module account to the recipient, with SendCoinsFromModuleToAccount.

Facts:

Funding for the community pool could be done from:

• GAMM module - fees in uosmo are charged for creating a pool
• Lockup module - Superfluid module sends only synthetic denoms for slashing.
  In SlashSyntheticLock: SlashingTokensFromLockByID is called on lockup module, and synthetic tokens are sent to CommunityPool.
• Mint module - Mints only uosmo coins, and sends the amount to the community pool
• Pool incentives module - amount of the minted coins are distributed to the community pool, so there is no problem because "uosmo" is configured for minting in the mint module.
• Token factory module - funds community pool with osmo tokens when being charged for creation of token-factory denom

But, there is a possibility to fund the community pool externally... This function is exposed on CLI and REST

Here, a depositor could send any type of denominations to community pool. We have not seen any validations that could exclude token-factory created denoms.

Conclusion:
Analysis showed that there were no modules that will fund the community pool with denominations other than native osmo.

However, it seems token factory created denoms could be sent to the pool.
If there are situations (that we could have possibly overseen, because of the large osmosis codebase) when the pool is funded with token factory-created denominations, we will not have the desired behavior.

We would need to:

• distribute the amount from the distribution module account (community pool) in a loop
• analyze the error code returned from the SendCoinsFromModuleToAccount, in order to ignore error code for "BeforeSend hook freezing the send" type of error, and to continue the distribution.

Context

We would like to investigate if it makes sense to flush pruning metadata to disk once it is added to memory. The concern is that if the node halts or crashess, the pruning heights are lost. As a result, these are never removed from the disk.

Originally posted by @ValarDragon in #140 (comment)

Acceptance Criteria

• Investigate if it is possible to pollute the disk with the current design
• If yes, create a PR with the fix
• Additional unit tests

Summary

We need a function in bank for SendManyCoins, and for SendCoinsFromModuleToManyAccount. These should mirror the SendCoins and SendCoinsFromModuleToAccount functions that already exist in bank. (SendCoins is in x/bank/keeper/send.go, SendCoinsFromModuleToAccount is in x/bank/keeper/keeper.go)

Heres the brief adaptation of each, bsaically taking the single case, and putting recipients in a loop:

func (k BaseKeeper) SendCoinsFromModuleToManyAccounts(
	ctx sdk.Context, senderModule string, recipientAddrs []sdk.AccAddress, amts []sdk.Coins,
) error {
        if len(recipientAddrs) != len(amts) {
		panic({MAKE AN ERROR HERE})
        }
	senderAddr := k.ak.GetModuleAddress(senderModule)
	if senderAddr == nil {
		panic(sdkerrors.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", senderModule))
	}

        for _, recipientAddr := range recipientAddrs {
	  if k.BlockedAddr(recipientAddr) {
		  return sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", recipientAddr)
	  }
       }

	return k.SendManyCoins(ctx, senderAddr, recipientAddrs, amts)
}

func (k BaseSendKeeper) SendManyCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddrs []sdk.AccAddress, amts []sdk.Coins) error {
	totalAmt := {sum up all amts}
	err := k.subUnlockedCoins(ctx, fromAddr, totalAmt)
	if err != nil {
		return err
	}
        fromAddrBech32 := fromAddr.String()
        for i, toAddr := range toAddrs {
       amt := amts[i]
	err = k.addCoins(ctx, toAddr, amt)
	if err != nil {
		return err
	}

	// Create account if recipient does not exist.
	//
	// NOTE: This should ultimately be removed in favor a more flexible approach
	// such as delegated fee messages.
	acc := k.ak.GetAccount(ctx, toAddr)
	if acc == nil {
		defer telemetry.IncrCounter(1, "new", "account")
		k.ak.SetAccount(ctx, k.ak.NewAccountWithAddress(ctx, toAddr))
	}

	ctx.EventManager().EmitEvent(		sdk.NewEvent(
			types.EventTypeTransfer,
			sdk.NewAttribute(types.AttributeKeyRecipient, toAddr.String()),
			sdk.NewAttribute(types.AttributeKeySender, fromAddrBech32),
			sdk.NewAttribute(sdk.AttributeKeyAmount, amt.String()),
		),
	)
}
}
	return nil
}

We should probably also switch SendCoins to use SendManyCoins instead, but thats pretty secondary.
The big gains here is it only bech32 encodes the fromAddr once, only does one subtract from the fromAddr, and only has one event per recipient, not 2 events per recipients.

While all of that sounds silly, its a very big impact at our scale. Standalone 50% events reduction, will be more paired with more osmosis changes. SendCoins is literally where ~all the computation time goes, this eliminates ~half the cost. (Actually more, because subtracting coins for weird reasons costed more than adding coins)

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

Background

As part of #240, we changed the function signature of the govtypes.NewMsgSubmitProposal from:

NewMsgSubmitProposal(content Content, initialDeposit sdk.Coins, proposer sdk.AccAddress)
to
NewMsgSubmitProposal(content Content, initialDeposit sdk.Coins, proposer sdk.AccAddress, IsExpedited bool)

in order to allow the creation of expedited proposals. However, because the IsExpedited boolean is required, it breaks all the existing usages of NewMsgSubmitProposal, which is annoying because many of the usages of this function are from other repos that we don't maintain such as ibc-go and wasmd, and as such we would be required to create forks of all of these.

Suggested Design

I suggest we either make the IsExpedited parameter optional and defaulted to false (I'm not sure if there's a way to do this in Go).

If that is not possible, we should revert the function signature to the original form, and just create a new function called govtypes.NewExpeditedMsgSubmitProposal that can be called when needed to create an expedited proposal

Acceptance Criteria

goals & criteria for success
(for example)

• all existing and new tests should pass

We are crypto exchange and trying to develop an application in golang that parses tx from different blockchains, including luna, osmosis, etc.
Now we run into a problem where osmosis has its own cosmos-sdk incompatible with githhub.com/cosmos-sdk used by other chains (like luna). As a result, we are not able to build osmosis parser into one application. In addition osmosis includes many new types that are not present in cosmos sdk. Since we need to build app that capable to parses all blockchains, building a separate parser just for osmosis is a lot of overhead and many duplication in other places we want to avoid

My question is, can we build osmosis tx parser without using osmosis's cosmos-sdk? Or can we somehow just register all osmosis types and their implementations that are present in osmosis package so we can parse its tx?

I've read osmosis source code, in particular, osmosisd application, and found the register process is quite involved, didn't find a way to isolate them.

Any help or advice will be greatly appreciated.

Summary

We should ensure that on MsgCreateValidator, if the self-delegation sent in the message isn't at least the provided MinSelfDelegation parameter, no new validator object is created.

Problem Definition

This, accompanied with a global minimum self delegation puts a non-zero cost barrier to spamming validator counts, which can cause notable complexity in other users codebase.

Proposal

Add a check right here:

Basically just do msg.Value.Amount.GTE(msg.MinSelfDelegation). If not, return an error.

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

Summary

There should be a governance proposal type to slash a validator. It would specify slash_percentage and validator address.

Problem Definition

This has a number of use cases. It allows blockchain communities to codify social standards that validators have to adhere to, and in the event they are 'caught' by the community, it allows them to slash them on-chain. (Without needing a hard fork)

A social slashing standard that has interest from the Osmosis community is essentially a "No Sybilling" condition, where they can all agree that if governance is presented with sufficiently persuasive evidence of a validator sybilling in order to game airdrops, newer staking reward designs, etc. they would be slashed for a certain factor. (With some carveout for cotenancy situations etc.). This is in part motivated by some validator-sybil-gameable decentralization ideas, cref https://commonwealth.im/osmosis/discussion/2036-proposal-per-validator-epoch-bonus-for-better-decentralization

I'm generally of the view that this is important for many chains, if validators commit malicious activities not yet thought about in advance, or on other chains. I propose we add this as a governance proposal type to the slashing module. Chains can simply choose not to add this governance proposal type to their governance router if they don't want this feature.

Proposal

Make a new governance proposal type in slashing with the following proto:

message SlashValidatorProposal {
  option (gogoproto.equal) = true;
  option (gogoproto.goproto_getters) = false;
  option (gogoproto.goproto_stringer) = false;

  string title = 1;
  string description = 2;
  string  validator_address = 3 [(cosmos_proto.scalar) = "cosmos.AddressString"];
  string slash_factor_percent = 4 [
    (cosmos_proto.scalar)  = "cosmos.Dec",
    (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec",
    (gogoproto.nullable)   = false
  ];
}

Heres an example of how you do custom governance proposal types if folks are interested:

• proto: https://github.com/osmosis-labs/osmosis/blob/main/proto/osmosis/pool-incentives/v1beta1/gov.proto
• gov.go: https://github.com/osmosis-labs/osmosis/blob/main/x/pool-incentives/types/gov.go
• keeper: https://github.com/osmosis-labs/osmosis/blob/main/x/pool-incentives/keeper/gov.go
• app.go: https://github.com/osmosis-labs/osmosis/blob/main/app/keepers.go#L263

Happy to help guide anyone interested in implementing this

To do this, you'd implement:

• the proto file here: https://github.com/cosmos/cosmos-sdk/tree/master/proto/cosmos/slashing/v1beta1
• gov.go here: https://github.com/cosmos/cosmos-sdk/tree/master/x/slashing/keeper
  • basically just calling https://github.com/cosmos/cosmos-sdk/blob/2f335a2b8cc4fcfa337c104823fab9f6ecb387da/x/slashing/keeper/keeper.go#L68 with distributionheight = ctx.BlockHeight()
• Another gov.go in the types folder

Reference SDK issue

There is a reference issue in the SDK here cosmos#10917 . I expect that I'm only going to track this one for the foreseeable future, since that one is blocked on a multi-month gov refactor.

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

Context

#140 , #187 refactored snapshot logic.

In these PRs, it was done so that the snapshot manager is not set (nil) if snapshot-interval is 0. However, snapshot manager is also responsible for state-sync. It might be the case that a node wants to catch up using state sync while turning off snapshots on their side.

Logs with this issue:

3:51AM INF Discovered new snapshot format=1 hash="\r��z��ܓ\x18Ws��\x19d�=ެ��=�\n\x05��\v���m" height=4329000 module=statesync
3:51AM INF Discovered new snapshot format=1 hash="\x008\x05(W���Y\b�b&��jڭ�\x10�,\a��o\x04c\a�E4" height=4327500 module=statesync
3:52AM INF VerifyHeader hash=6E992F698F0D454B2AC5458CB1C467BF9B393A11C86025CA986DA1235DF23238 height=4329001 module=light
3:52AM INF VerifyHeader hash=98CA996C1ED5ECA1731CEACA67EE08B03755C690F38745E50146AFAF2455360B height=4329002 module=light
3:52AM INF Offering snapshot to ABCI app format=1 hash="\r��z��ܓ\x18Ws��\x19d�=ެ��=�\n\x05��\v���m" height=4329000 module=statesync
3:46AM ERR snapshot manager not configured
3:46AM ERR State sync failed err="state sync aborted" module=statesync

We should decouple the logic so that the snapshot manager is created independently from the snapshot-inteval value in app.toml

Acceptance Criteria

• state sync is enabled independently from the value of snapshot-interval in app.toml
• unit tests added
• snapshots and their configuration works as expected

Description

An argument can be made for the need of a logical binary merkle store that is primary used for state-sync only, i.e. no need for light client proofs.

Theres lots of data we want only state syncable, and not light client provable in blockchains. If you want a state sync only logical store, you can basically make a merkle-ized BTree, but with fan-out factor of 1024, or 1024 * 1024. This is similar to what Dropbox does. This provides very low overhead and you get a final hash you can include in the root multi-store.

However, we want to utilize a different physical storage for the underlying DB, such as sqlite as opposed to IAVL. This is to take advantage of performance of such databases and the layout of data in storage that will help aid in various retrieval and storage patterns.

Modules should be able to declare the need to use such a store for various aspects of its state, if not for the entire state. To facilitate this we need to perform the following:

Tasks

• Implement a merkle BTree as a KVStore
• Support the ability to provide multiple physical databases in config.toml
• Define extension APIs to allow modules to use and declare separate store types
• Any changes necessary to the root multistore API

This PR added amino annotations for codegen.

If we don't want to take the entire PR, I suggest we at least add this proto file

this PR in Osmosis depends on this issue

we need to keep in sync with using full-scoped interface names in protos (already merged in cosmos-sdk)

• reference PR
• issue describing problem
• new updated docs with required changes

As pointed out by Hathor on Discord:

https://lcd-osmosis.blockapsis.com/cosmos/tx/v1beta1/txs?events=message.action=%27superfluid_delegate%27 <-- Defaults to 100
https://lcd-osmosis.blockapsis.com/cosmos/tx/v1beta1/txs?events=message.action=%27superfluid_delegate%27&pagination.limit=50 <-- Sets it to 50 tx in a page
https://lcd-osmosis.blockapsis.com/cosmos/tx/v1beta1/txs?events=message.action=%27superfluid_delegate%27&pagination.limit=500 <-- Still at 100

This behavior is not just on publicly available endpoints, it is default behavior. It should also be known that when checking this via CLI, you are able to sift through x / y pages to see all txs with the requested event, so this behavior is only happening when querying with LCD

The only way around this using LCD is to first do one request to find the pagination.total, then loop through with &pagination.offset=100 etc until the pagination.offset is greater than or equal to the pagination total. RPC allows for any limit to be set, but this requires knowing the abci_query path and encodes which is also not a great method.

Edit:

Also, attempting to increase the limit on the CLI does not work (seems like the limit flag doesn't work as intended)
osmosisd query txs --events="message.action=superfluid_delegate" --limit=30000

So, because of differences in the ABCI interface, having abci++ / tendermint v0.37.x will require forking IBC-go

What should we do here?

I think the options are:

• Use SDK 47 without depinject features, while still using tendermint v0.37.x
• Remove tendermint 37 from our main cosmos-sdk branch

But I figure that there could be other things I haven't thought of.

Hi, Just resolved issues with the fast node enabled 42 series sdk.

If someone can please make a protected branch for v3, then I can PR those changes into it, and we will have a protected branch for the earliest versions of Osmosis, v1-v3.

Thanks!

Context

I think this has to do with -race tests being slower than regular and, as a result, timing out. The test passes on my local machine without a timeout. So it might just mean that we need to find the right timeout value for the CI host machine.

We can tweak the timeout value here:

Feel free to merge this PR, I can find the right value for the timeout in a separate PR

Originally posted by @p0mvn in #158 (comment)

I searched the repository but could not find any example. Where can I find the steps:

1. generate a private key
2. derive public from private key
3. derive address from public key
   Thanks

System information

Osmosis version: 15.0.0
OS & Version: macOS
Commit hash: ff18d8244fcda7313ec951fb1b3bee8369b8316b

Expected behaviour

I just created mainnet prop 489 with the following command which specifies --is-expedited false:

osmosisd tx gov submit-proposal \
  --title "Set token creation fee to zero; introduce an alternative spam-prevention mechanism" \
  --description "$(cat workspace/proposal.md)" \
  --type Text \
  --deposit 1600000000uosmo \
  --is-expedited false \
  --from myaccount \
  --gas auto --gas-adjustment 1.4 --gas-prices 0.0025uosmo

Actual behaviour

A tx is generated with "is_expedited": true:

My speculation is this has to do with --is-expedited false vs --is-expedited=false. Maybe using a space vs using an equal mark is different. Not sure.

cc @p0mvn for test snapshot with pruning failing here, it seems likely to be unrelated to this PR

Originally posted by @ValarDragon in #178 (comment)