TEST-40: Remove all dependencies on ibc-v2 #14

shellvish · 2022-05-16T19:33:54Z

Summary

Removed tendermint-spn and cosmos-sdk CLI, both of which used IBC-v2.

Specifically:

Added StrideApp to app.go (replaces previous generic App object). Created necessary helpers to make this a valid implementation (e.g. GetIBCKeeper, GetStakingKeeper, etc)
Removed OpenAPI routes for the app (I don't believe we were using these)
Added encoding.go, which is inspired by QS's implementation. Very similar to Osmosis's implementation (virtually identical, but Osmosis defines structs in a separate file). I'm unsure exactly what this is doing, but I believe encoding transactions to be sent over (using the "Amino" protobuf protocol?)
Created genaccounts.go which defines AddGenesisAccountCmd (from Osmosis + QS), which adds a command line argument to add accounts to genesis (with vesting! although unclear if they receive voting rights)
Added root.go file that defines a cmd line interface (using Cobra)

PLEASE tell me if you want more comments anywhere in this commit, I'm always happy to improve code readability

Test plan

ignite chain serve now works properly, and we're able to get a functioning chain!

asalzmann · 2022-05-16T21:01:41Z

At a high level, is the approach here to emulate the interchain-accounts-demo approach by swapping out the CLI?

shellvish · 2022-05-17T01:59:26Z

basically yeah, although it's mostly inspired by Osmosis + QS (although both of them are p similar to the implementation in interchain-accounts-demo

asalzmann

Overall lgtm! Just a few small comments

asalzmann · 2022-05-17T09:16:41Z

stride/go.mod

@@ -3,29 +3,29 @@ module github.com/Stride-labs/stride
 go 1.16

 require (
-	github.com/cosmos/cosmos-sdk v0.45.3
+	github.com/cosmos/cosmos-sdk v0.45.4


Looks like there are a bunch of versioning updates in this file, were these auto-generated?

Ah, yes, although they're unnecessary. I previously thought if we updated the cosmos-sdk it would resolve our v2/v3 bugs, and then I never downgraded again. We can revert this if desired

asalzmann · 2022-05-17T10:04:33Z

stride/app/export.go

@@ -76,20 +76,23 @@ func (app *App) prepForZeroHeightGenesis(ctx sdk.Context, jailAllowedAddrs []str

 	// withdraw all validator commission
 	app.StakingKeeper.IterateValidators(ctx, func(_ int64, val stakingtypes.ValidatorI) (stop bool) {
-		_, err := app.DistrKeeper.WithdrawValidatorCommission(ctx, val.GetOperator())
-		if err != nil {


why remove the error checking here?

I had the same Q. Seems like it was inspired by Osmosis' implementation (QS does the same thing). Though I wonder if both those implementations are outdated relative to the error checking that ignite provides by default.

Hard to know whether we need the handling here. Seems like the SDK does some error handling but not a ton

Actually I think I like Ignite's version better -- I just checked the git blame and see that Osmosis' code for this section was written 2 years ago haha

https://github.com/osmosis-labs/osmosis/blame/e3f09f3734e8b325df10a24af587084b3965cd46/app/export.go#L76

asalzmann · 2022-05-17T10:09:14Z

stride/cmd/strided/genaccounts.go

+	"github.com/cosmos/cosmos-sdk/server"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
+	authvesting "github.com/cosmos/cosmos-sdk/x/auth/vesting/types"


More info on this here https://docs.cosmos.network/main/modules/auth/05_vesting.html#intro-and-requirements

Importantly, "For all vesting accounts, the owner of the vesting account is able to delegate and undelegate from validators, however they cannot transfer coins to another account until those coins are vested."

If vest investors on chain, we should probably use period vesting

"Periodic vesting, where coins begin to vest at ST and vest periodically according to number of periods and the vesting amount per period. The number of periods, length per period, and amount per period are configurable. A periodic vesting account is distinguished from a continuous vesting account in that coins can be released in staggered tranches. For example, a periodic vesting account could be used for vesting arrangements where coins are relased quarterly, yearly, or over any other function of tokens over time."

Good find and I think we should use this IFF we can find a way to restrict them from delegating. Seems like vesting is written so that all vesting options allow delegation (and thus I believe the accrual of staking rewards, which we do NOT want to allow). The other option is something manual e.g. we hold all the tokens and transfer some to investors once a month (not too difficult, though admittedly not as "clean" as something like a vesting module)

One option would be to fork the vesting module, but I am hesitant to open that can of worms (high downside, low upside)

Agreed, I'm a bit worried about forking it too. We can ask investors their preferences here, it's possible everyone is fine just signing a contract saying they won't stake

asalzmann · 2022-05-17T10:14:51Z

stride/cmd/strided/root.go

+	//   own app.toml to override, or use this default value.
+	//
+	// In simapp, we set the min gas prices to 0.
+	srvCfg.MinGasPrices = "0stake"


Here's how Osmosis allows for gas prices to be 0! We should take not of this. @vish-stride can you add a ticket to JIRA to track this and a TODO here referencing the ticket?

Looks like this is a local parameter set on individual nodes (unclear if we need to make any updates to our codebase to enable 0 fee transactions)

Again, QS chose to do this thru CLI instead of in-code. I wonder what is driving this design decision for them across the entire app

https://github.com/ingenuity-build/quicksilver/blob/main/cmd/quicksilverd/root.go#L228
https://github.com/ingenuity-build/quicksilver/blob/969b1d972940b67cf23c7a2d534eb87fabf51679/app/test_helpers.go#L39

riley-stride

Overall this looks good. Sorry you had to grind through this one and thanks for sticking it out. Added some light comments but overall looks like a lot of imported logic from QS and Osmosis.

One overarching Q that jumps out (evidenced by Aidan's Q about your removing an error check) is whether we should put more faith is Osmosis' live code (which was however written long ago) or Ignite's scaffolded code (likely less battle tested, but newer)

…pendencies

…rsion Switch version to recommended_version, add compatible_versions

Added fields, changed time type

removed CLI import and tendermint-spn, built own command line interface

b6ed899

shellvish requested review from riley-stride and asalzmann May 16, 2022 19:33

asalzmann approved these changes May 17, 2022

View reviewed changes

This was referenced May 17, 2022

TEST-18 Add Epochs and Block actions #15

Merged

TEST-7: add exchange rate #17

Closed

riley-stride approved these changes May 18, 2022

View reviewed changes

shellvish added 3 commits May 17, 2022 23:35

add TODO around zero gas transactions

bf611d4

Merge remote-tracking branch 'origin/main' into TEST-40-fix-ibc-v2-de…

12a5432

…pendencies

resolve merge conflicts

28e47ab

shellvish merged commit c5a50d2 into main May 18, 2022

asalzmann mentioned this pull request May 19, 2022

Add Makefile, bash scripts to test multiple nodes #20

Merged

riley-stride pushed a commit that referenced this pull request Sep 5, 2022

Merge pull request #14 from osmosis-labs/dev/switch_to_recommended_ve…

2d8f6dc

…rsion Switch version to recommended_version, add compatible_versions

asalzmann pushed a commit that referenced this pull request Jan 25, 2024

Merge pull request #14 from Stride-Labs/nit_proto_updates

e8960ba

Added fields, changed time type

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TEST-40: Remove all dependencies on ibc-v2 #14

TEST-40: Remove all dependencies on ibc-v2 #14

shellvish commented May 16, 2022 •

edited

Loading

asalzmann commented May 16, 2022

shellvish commented May 17, 2022

asalzmann left a comment

asalzmann May 17, 2022

shellvish May 18, 2022

asalzmann May 17, 2022

riley-stride May 18, 2022

riley-stride May 18, 2022

asalzmann May 17, 2022

riley-stride May 18, 2022

riley-stride May 18, 2022 •

edited

Loading

shellvish May 18, 2022

asalzmann May 17, 2022

asalzmann May 17, 2022

riley-stride May 18, 2022

riley-stride left a comment

TEST-40: Remove all dependencies on ibc-v2 #14

TEST-40: Remove all dependencies on ibc-v2 #14

Conversation

shellvish commented May 16, 2022 • edited Loading

asalzmann commented May 16, 2022

shellvish commented May 17, 2022

asalzmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riley-stride May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riley-stride left a comment

Choose a reason for hiding this comment

shellvish commented May 16, 2022 •

edited

Loading

riley-stride May 18, 2022 •

edited

Loading