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

feat: add LSM to the SDK's v0.45.16-ics-lsm branch #16747

Merged

Conversation

riley-stride
Copy link

@riley-stride riley-stride commented Jun 28, 2023

Description

Add Iqlusion's LSM to the SDK in v0.45.16-ics-lsm for use in the upcoming Cosmoshub v11 release.

Please see this Iqlusion forum post for a full description of the LSM deployment plan on the Cosmoshub.

TLDR; On this branch:

  • ICS changes are managed by the ICS/Hub team
  • LSM changes are managed by Iqlusion team
  • SDK changes (bugfixes expected only) are managed by the SDK team

Credit to @sampocs for a lot of hard work combining ICS + LSM 🙌


Disclaimers for reviewers:

  1. There's one last stubborn failing unit test: TestGRPCServer_Reflection (LOC). The unit test complains that the TokenizeShareRecord struct isn't accessible, but it's defined here.

Error log

--- FAIL: TestIntegrationTestSuite (105.88s)
    server_test.go:45: setting up integration test suite
    network.go:172: acquiring test network lock
    network.go:177: created temporary directory: /var/folders/m5/12pnd05500n56z31y9rtmmsr0000gn/T/TestIntegrationTestSuite2846596830/001/chain-D7Uv8k3239444353
    network.go:186: preparing test network...
    network.go:379: starting test network...
    network.go:384: started test network
    --- FAIL: TestIntegrationTestSuite/TestGRPCServer_Reflection (89.70s)
        server_test.go:126: 
            	Error Trace:	/Users/rileyedmunds/iqlusion/cosmos-sdk/server/grpc/server_test.go:126
            	Error:      	Received unexpected error:
            	            	file "cosmos/staking/v1beta1/query.proto" included an unresolvable reference to ".cosmos.staking.v1beta1.TokenizeShareRecord"
            	Test:       	TestIntegrationTestSuite/TestGRPCServer_Reflection
  1. Linting fails on this branch. However, linting also fails on the v0.45.16-ics branch. We believe we've resolved all lint issues related to the diffs that are added in this pull request.

  2. MsgCancelUnbondingDelegation was part of the original LSM design since LSM was originally written on SDK46. We ported it over here as a safety feature, but are open to removing it if the SDK team prefers keeping 45 fully feature-frozen.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

sampocs and others added 2 commits August 3, 2023 14:27
@alexanderbez alexanderbez deleted the branch cosmos:feature/v0.45.x-ics-lsm August 3, 2023 20:18
@alexanderbez alexanderbez reopened this Aug 3, 2023
@alexanderbez alexanderbez changed the base branch from v0.45.16-ics-lsm to feature/v0.45.x-ics-lsm August 3, 2023 22:27
checkValidatorBondShares(validatorBAddress, sdk.ZeroInt())

// Redelegate partially from A to B - it should remove the bond shares from the source validator
_, err = msgServer.BeginRedelegate(sdk.WrapSDKContext(ctx), &types.MsgBeginRedelegate{
Copy link

@ajeet97 ajeet97 Aug 10, 2023

Choose a reason for hiding this comment

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

I was wondering if the delegation was flagged as validator bond for validator B before redelegation.
And then after redelegation, should the validator B's bond shares increase equal to redelegation amount?
cc: @xlab

Copy link
Contributor

Choose a reason for hiding this comment

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

great catch! Resolved here

Copy link

Choose a reason for hiding this comment

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

Awesome 🎉

Copy link

@mpoke mpoke left a comment

Choose a reason for hiding this comment

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

LGTM. Great work @riley-stride @sampocs

@mpoke
Copy link

mpoke commented Aug 10, 2023

nit: could somebody run make format?

@mpoke
Copy link

mpoke commented Aug 10, 2023

Also, are we sure we cannot get more tests passing? @alexanderbez Are any of the CI checks failing a problem?

@zmanian
Copy link
Member

zmanian commented Aug 13, 2023

LFG

Copy link

@asalzmann asalzmann left a comment

Choose a reason for hiding this comment

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

LGTM

@alexanderbez alexanderbez merged commit 201528c into cosmos:feature/v0.45.x-ics-lsm Aug 15, 2023
24 of 31 checks passed
@julienrbrt
Copy link
Member

I haven't reviewed this, but is it expected that those sims are failing:

See snippet
$ go test ./simapp -run TestFullAppSimulation -Enabled=true -NumBlocks=50 -Genesis= -Verbose=true -Commit=true -Seed=32 -Period=10 -v -timeout 24h
....
Starting the simulation from time Mon Aug 22 13:36:50 UTC 8839 (unixtime 216784906610)
Simulating... block 20/50, operation 250/259. simulation halted due to panic on block 20
Logs to writing to /home/julien/.simapp/simulations/2023-08-15_18:44:55.log
--- FAIL: TestFullAppSimulation (6.19s)
panic: negative coin amount [recovered]
        panic: negative coin amount [recovered]
        panic: negative coin amount

goroutine 51 [running]:
testing.tRunner.func1.2({0x139b820, 0x1da88e0})
        /usr/local/go/src/testing/testing.go:1545 +0x238
testing.tRunner.func1()
        /usr/local/go/src/testing/testing.go:1548 +0x397
panic({0x139b820?, 0x1da88e0?})
        /usr/local/go/src/runtime/panic.go:914 +0x21f
github.com/cosmos/cosmos-sdk/x/simulation.SimulateFromSeed.func2()
        /home/julien/projects/cosmos/cosmos-sdk/x/simulation/simulate.go:146 +0xd3
panic({0x139b820?, 0x1da88e0?})
        /usr/local/go/src/runtime/panic.go:920 +0x270
github.com/cosmos/cosmos-sdk/types.DecCoins.Sub(...)
        /home/julien/projects/cosmos/cosmos-sdk/types/dec_coin.go:306
github.com/cosmos/cosmos-sdk/x/distribution/keeper.Keeper.WithdrawValidatorCommission({{0x1dafc00, 0xc00011f1b0}, {0x7fd474320f58, 0xc00018f6d0}, {{0x7fd474320f58, 0xc00018f6d0}, 0xc00011aa18, {0x1dafc00, 0xc00011f1e0}, {0x1dafc50, ...}, ...}, ...}, ...)
        /home/julien/projects/cosmos/cosmos-sdk/x/distribution/keeper/keeper.go:119 +0x890
github.com/cosmos/cosmos-sdk/x/distribution/keeper.RegisterInvariants.CanWithdrawInvariant.func2.1(0x15c4600?, {0x1dd04f8, 0xc001915440})
        /home/julien/projects/cosmos/cosmos-sdk/x/distribution/keeper/invariants.go:80 +0xd8
github.com/cosmos/cosmos-sdk/x/staking/keeper.Keeper.IterateValidators({{0x1dafc00, 0xc00011f190}, {0x7fd474320f58, 0xc00018f6d0}, {0x1dc0458, 0xc000304ea0}, {0x7fd474320fc8, 0xc000217340}, {0x1dc9770, 0xc000156a80}, ...}, ...)
        /home/julien/projects/cosmos/cosmos-sdk/x/staking/keeper/alias_functions.go:23 +0x1ec
github.com/cosmos/cosmos-sdk/x/distribution/keeper.RegisterInvariants.CanWithdrawInvariant.func2({{0x1dbf620, 0x28ebfe0}, {0x1dc8c10, 0xc00357db00}, {{0x0, 0x0}, {0x15e51cb, 0xe}, 0x14, {0x0, ...}, ...}, ...})
        /home/julien/projects/cosmos/cosmos-sdk/x/distribution/keeper/invariants.go:79 +0x51c
github.com/cosmos/cosmos-sdk/x/crisis/keeper.Keeper.AssertInvariants({{0xc0002a0c80, 0xb, 0x10}, {{0x7fd474320f58, 0xc00018f6d0}, 0xc00011aa18, {0x1dafc00, 0xc00011f1e0}, {0x1dafc50, 0xc00011f250}, ...}, ...}, ...)
        /home/julien/projects/cosmos/cosmos-sdk/x/crisis/keeper/keeper.go:79 +0x41d
github.com/cosmos/cosmos-sdk/x/crisis.EndBlocker({{0x1dbf620, 0x28ebfe0}, {0x1dc8c10, 0xc00357db00}, {{0x0, 0x0}, {0x15e51cb, 0xe}, 0x14, {0x0, ...}, ...}, ...}, ...)
        /home/julien/projects/cosmos/cosmos-sdk/x/crisis/abci.go:20 +0x218
github.com/cosmos/cosmos-sdk/x/crisis.AppModule.EndBlock(...)
        /home/julien/projects/cosmos/cosmos-sdk/x/crisis/module.go:165
github.com/cosmos/cosmos-sdk/types/module.(*Manager).EndBlock(_, {{0x1dbf620, 0x28ebfe0}, {0x1dc8c10, 0xc00357db00}, {{0x0, 0x0}, {0x15e51cb, 0xe}, 0x14, ...}, ...}, ...)
        /home/julien/projects/cosmos/cosmos-sdk/types/module/module.go:512 +0x1eb
github.com/cosmos/cosmos-sdk/simapp.(*SimApp).EndBlocker(...)
        /home/julien/projects/cosmos/cosmos-sdk/simapp/app.go:436
github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).EndBlock(0xc000148700, {0x28ebfe0?})
        /home/julien/projects/cosmos/cosmos-sdk/baseapp/abci.go:203 +0x1fd
github.com/cosmos/cosmos-sdk/x/simulation.SimulateFromSeed({0x1dce3f0?, 0xc0005829c0?}, {0x1dab540?, 0xc00008e030}, 0x0?, _, _, {0xc0002a6900, 0x1f, 0x30}, ...)
        /home/julien/projects/cosmos/cosmos-sdk/x/simulation/simulate.go:185 +0x17d8
github.com/cosmos/cosmos-sdk/simapp.TestFullAppSimulation(0xc0005829c0)
        /home/julien/projects/cosmos/cosmos-sdk/simapp/sim_test.go:74 +0x5d4
testing.tRunner(0xc0005829c0, 0x1c27e98)
        /usr/local/go/src/testing/testing.go:1595 +0xff
created by testing.(*T).Run in goroutine 1
        /usr/local/go/src/testing/testing.go:1648 +0x3ad
FAIL    github.com/cosmos/cosmos-sdk/simapp     6.216s
FAIL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.