New branch sdk47 #817

sontrinh16 · 2023-04-02T01:35:24Z

Closes: #852

Description

-remove the third_party folder
-Using sdk47, ibc-go v7
-Switch to forked vesion for ibc-go to bypass the passing of pointer to empty struct in IBCKeeper.NewKeeper
-Switch to forked vesion for sdk47 to bypass valset check when init chain for consumer app, modify expected Slash func parameters in stakingkeeper interface so consumer app can use is consumerkeeper as a stakingkeeper

Type of change

If you've checked more than one of the first three boxes, consider splitting this PR into multiple PRs!

Move sdk version to v0.47.1
Fix all dependencies issues
Refactor code base on said changes
Remove the legacy-ibc-testing ( base on the TO DO note )
Add any migrations logic if needed

Versioning Implications

This PR will affect semantic versioning as defined for ICS

If the above box is checked, which version should be bumped?

MAJOR: Consensus breaking changes to both the provider and consumers(s), including updates/breaking changes to IBC communication between provider and consumer(s)
MINOR: Consensus breaking changes which affect either only the provider or only the consumer(s)
PATCH: Non consensus breaking changes

Targeting

Please select one of the following:

This PR is only relevant to main
This PR is relevant to main, and should also be back-ported to ____ (ex: v1.0.0 and v1.1.0)
This PR is only relevant to ____ (ex: v1.0.0, v1.1.0, and v1.2.0)

…nterchain-security into new_branch_sdk47

faddat · 2023-04-20T07:27:03Z

hey, cool! so, this is improving I think.

faddat · 2023-04-20T07:28:01Z

@sontrinh16 can you please add the removal of the third_party folder to the description?

https://github.com/mvdan/github-actions-golang/blob/master/README.md

mpoke · 2023-04-20T12:04:15Z

@sontrinh16 please stop making general PRs, they need to be focused on solving one or a limited amount of things at once. This is very hard to get reviewed and consume more time than you may expect.

The work on upgrading the consumer CCV module to SDK v0.47 is tracked in this issue #852. As this upgrade entails a lot of changes that may impact multiple teams, it will be coordinated by the core ICS team. For contributing to this work, please participate in discussions in the open issue.

tac0turtle · 2023-04-20T13:44:00Z

This pr is valid, it contains the things needed to upgrade this repository as the issue explains. If you are asking for the work to be split across a few prs, which it seems based off the comment then lets document this.

Contributors are crucial to our ecosystem and closing valid prs doesnt seem like you want people to work on this repo or issue, if this is the case then it should be said that this issue will or should be handled by the team. Notional has updated a few repositories to 0.47 from my knowledge so its worth taking some advice or help from them

jtremback · 2023-04-20T22:54:47Z

.github/workflows/gosec.yml

@@ -1,20 +0,0 @@
-name: gosec


Why is this being deleted?

jtremback · 2023-04-20T22:57:04Z

docs/docs/consumer-development/consumer-chain-governance.md

@@ -3,23 +3,3 @@ sidebar_position: 2
 ---

 # Consumer Chain Governance
-


Why is this being deleted?

jtremback · 2023-04-20T22:57:40Z

docs/docs/features/key-assignment.md

@@ -41,7 +41,7 @@ consumerd tendermint show-validator # {"@type":"/cosmos.crypto.ed25519.PubKey","
 Then, make an `assign-consensus-key` transaction on the provider chain in order to inform the provider chain about the consensus key you will be using for a specific consumer chain.

 ```
-gaiad tx provider assign-consensus-key <consumer-chain-id> '<pubkey>' --from <tx-signer> --home <home_dir> --gas 900000 -b block -y -o json
+gaiad tx provider assign-consensus-key <consumer-chain-id> '<pubkey>' --from <tx-signer> --home <home_dir> --gas 900000 -b sync -y -o json


Oh, did they change the flag in 47?

jtremback · 2023-04-20T22:58:01Z

go.mod

 	github.com/spf13/cast v1.5.0
-	github.com/spf13/cobra v1.7.0
+	github.com/spf13/cobra v1.6.1


Why the downgrade?

jtremback · 2023-04-20T22:59:24Z

go.mod

-	github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1
-	github.com/tendermint/tendermint => github.com/cometbft/cometbft v0.34.27
-	google.golang.org/grpc => google.golang.org/grpc v1.33.2
+	github.com/cosmos/cosmos-sdk => github.com/notional-labs/cosmos-sdk v0.47.2-0.20230414131805-781d3665d7a0


We're going to need to get these into mainline SDK. Can you link your PR on SDK?

jtremback · 2023-04-20T22:59:36Z

go.mod

-	github.com/tendermint/tendermint => github.com/cometbft/cometbft v0.34.27
-	google.golang.org/grpc => google.golang.org/grpc v1.33.2
+	github.com/cosmos/cosmos-sdk => github.com/notional-labs/cosmos-sdk v0.47.2-0.20230414131805-781d3665d7a0
+	github.com/cosmos/ibc-go/v7 => github.com/notional-labs/ibc-go/v7 v7.0.0-rc0.0.20230417042817-8072b1e9aabc


Same. Can you link your PR on IBC?

jtremback · 2023-04-20T23:01:20Z

x/ccv/consumer/ibc_module.go

@@ -38,7 +39,7 @@ func (am AppModule) OnChanOpenInit(

 	// ensure provider channel hasn't already been created
 	if providerChannel, ok := am.keeper.GetProviderChannel(ctx); ok {
-		return "", sdkerrors.Wrapf(types.ErrDuplicateChannel,
+		return "", errorsmod.Wrapf(types.ErrDuplicateChannel,


What is this change for?

jtremback · 2023-04-20T23:03:04Z

x/ccv/consumer/ibc_module.go

-	counterparty channeltypes.Counterparty,
-	counterpartyVersion string,
+func (AppModule) OnChanOpenTry(
+	_ sdk.Context,


Please put formatting changes in a separate PR. We merged Notional's linting and formatting PRs a couple weeks ago. Why wasn't this changed then?

jtremback · 2023-04-20T23:04:57Z

x/ccv/consumer/keeper/distribution.go

+				SourcePort:       transfertypes.PortID,     // packet destination port is now the source
+				SourceChannel:    ch,                       // packet destination channel is now the source
+				Token:            token,                    // balance of the coin
+				Sender:           tstProviderAddr.String(), // recipient is the address in the Evmos chain


These comments look wrong

jtremback · 2023-04-20T23:13:23Z

x/ccv/consumer/keeper/distribution.go

@@ -211,7 +213,7 @@ func (k Keeper) GetEstimatedNextFeeDistribution(ctx sdk.Context) types.NextFeeDi
 	totalTokens := sdk.NewDecCoinsFromCoins(total...)
 	// truncated decimals are implicitly added to provider
 	consumerTokens, _ := totalTokens.MulDec(frac).TruncateDecimal()
-	providerTokens := total.Sub(consumerTokens)
+	providerTokens := total.Sub(consumerTokens[0])


What does this do?

jtremback · 2023-04-20T23:15:56Z

x/ccv/consumer/keeper/keeper.go

 		return "", false
 	}
-	return string(channelIdBytes), true
+	return string(channelIDBytes), true


Why wasn't this changed in the formatting and linting PRs from Notional that we merged a few weeks ago? Shouldn't be in this PR.

jtremback · 2023-04-20T23:17:50Z

x/ccv/consumer/keeper/validators_test.go

@@ -25,7 +26,7 @@ func TestApplyCCValidatorChanges(t *testing.T) {
 	// utility functions
 	getCCVals := func() (vals []types.CrossChainValidator) {
 		vals = consumerKeeper.GetAllCCValidator(ctx)
-		return
+		return vals


No formatting in this PR please

faddat · 2023-04-21T09:18:34Z

Sorry sir it's been closed thank you for being the only informal team member to give actionable feedback.

We will start from main.

mpoke · 2023-04-21T10:35:38Z

This pr is valid, it contains the things needed to upgrade this repository as the issue explains. If you are asking for the work to be split across a few prs, which it seems based off the comment then lets document this.

Contributors are crucial to our ecosystem and closing valid prs doesnt seem like you want people to work on this repo or issue, if this is the case then it should be said that this issue will or should be handled by the team. Notional has updated a few repositories to 0.47 from my knowledge so its worth taking some advice or help from them

We're happy to take advice/help from contributors. In order for us to accept contributions, we need contributors to follow our contributing guidelines (see CONTRIBUTING.md in the root of the repo). The reason for this is, while others may submit these pull requests, our team is ultimately responsible for maintaining the code that gets merged and eventually released, meaning that our team needs as much context as possible on the nature of all of the changes being merged.

As such, for large/complex changes, we absolutely require up-front discussion in issues/GitHub discussions. In some cases, like this change, it would be preferable to even have synchronous design sessions to socialize a collective understanding of the nature of incoming changes prior to the work even starting.

Then, when large changes are submitted, we need those changes to be easy for our team to review. This means that changes either need to be submitted in multiple small PRs (300-500 lines per PR max) that build on top of one another, or that large PRs need to be structured such that they can be reviewed commit by commit (300-500 lines per commit max), with each commit performing a single logical task.

And finally, if someone from our team is assigned to a particular issue, we will most likely opt to not accept unsolicited PRs that attempt to address those same issues. As a general rule, if someone else is assigned to an issue and you would like to take it over, please reach out to that person and ask whether you can either collaborate with them on it or take it over.

We're of course open to collaborating in future, but future collaboration needs to be mindful of the above.

cc @tac0turtle @sontrinh16 @faddat

faddat · 2023-04-25T02:06:25Z

Folks, let me make very clear please, and as politely as possible, you don't own this repository, you work on it.

You repeatedly told us that changes to the SDK would not be necessary. We have attempted to get engaged in slack and the like, we have attempted to set up more routine calls with you, we have changed the staffing of this work to attempt to better cater to the needs of informal systems, but the reality is you just keep doing it, and you know, it's really a very perfect scenario for you guys to keep lifting our work over and over and over and over and over again for you to ask us to do the work, have the whole ecosystem. Very excited about getting 47 on ICS, and then simply say that you won't merge it.

Here is where we can have discussions in public:

https://t.me/+cDuEIPMPBXVkNDU1

and of course if you choose not to join well, that should tell the public some stuff, and if you try to say that you're responsive and slack but you're not, I'll publish that as well because the community deserves transparency.

The community should know that you have laid sole claim to this repository.

Now please keep in mind that that governance server is tightly moderated. No one is going to harass you or do nasty stuff there like the previous ones, and if they do they magically disappear.

I am not trying to be nasty here, I am attempting to tell you what is necessary to actually do multi-stakeholder work.

faddat and others added 30 commits March 24, 2023 00:36

use 45.15

9804a91

remove ignite

3c3f7bc

fix ibc_testing

2f1a331

fumpt

7e4383f

format code

1f2f288

introduce revive

a6c879c

Merge branch 'main' into revive-lints

c8c6196

unused recievers

21230b0

additional gofumpt rules

63281e1

if-return

8bb531b

unused recievers

cb277f1

fully linted with strict settings and primed for upgrades

90c0b60

gonna need testutil

0382161

use gosec as part of golang-ci

e9c596c

introduce typically used linters

e4d8b99

lint code including the conversion away from the ignite cli

25f57ed

lint code including the conversion away from the ignite cli

c62529d

remove ioutil

0c7db80

only one params folder

8501326

fixing err

dcf75c0

add forked staking proto

4da470a

fix x/comsumer

497608f

fix x folder

89dae6f

add staking infraction type

0a3d615

implement missing func

e8ae32d

remove res

571d4ee

use app params makeEncodingConfig

537005c

fix register interface

b7407ee

try ibc v7 testing

b5c980e

Merge branch 'main' into new_branch_sdk47

db7a841

sontrinh16 and others added 7 commits April 20, 2023 13:19

Merge branch 'new_branch_sdk47' of https://github.com/notional-labs/i…

aa56a1c

…nterchain-security into new_branch_sdk47

minor

d7774a7

remove unneeded proto deps and build with many fewer

b676e05

remove even more proto code

8d4c413

make protos match the v7.0.x branch exactly

2becc4e

make protos match exactly

e691f79

remove proto files completely

6f46e0c

Create go-tests.yml

1ecf028

https://github.com/mvdan/github-actions-golang/blob/master/README.md

faddat mentioned this pull request Apr 20, 2023

feat: changes for ics cosmos/ibc-go#3496

Closed

9 tasks

mpoke closed this Apr 20, 2023

jtremback reviewed Apr 20, 2023

View reviewed changes

.github/workflows/gosec.yml

@@ -1,20 +0,0 @@

name: gosec

Copy link

Contributor

jtremback Apr 20, 2023

Choose a reason for hiding this comment

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

Why is this being deleted?

jtremback reviewed Apr 20, 2023

View reviewed changes

go.mod

github.com/spf13/cast v1.5.0

github.com/spf13/cobra v1.7.0

github.com/spf13/cobra v1.6.1

Copy link

Contributor

jtremback Apr 20, 2023

Choose a reason for hiding this comment

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

Why the downgrade?

jtremback reviewed Apr 20, 2023

View reviewed changes

jtremback mentioned this pull request Apr 24, 2023

fix: changes to sdk for interchain security cosmos/cosmos-sdk#15917

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New branch sdk47 #817

New branch sdk47 #817

sontrinh16 commented Apr 2, 2023 •

edited

Loading

faddat commented Apr 20, 2023

faddat commented Apr 20, 2023 •

edited

Loading

mpoke commented Apr 20, 2023

tac0turtle commented Apr 20, 2023

jtremback Apr 20, 2023

jtremback Apr 20, 2023

jtremback Apr 20, 2023

pysel Apr 21, 2023

jtremback Apr 20, 2023

jtremback Apr 20, 2023

jtremback Apr 20, 2023

jtremback Apr 20, 2023

jtremback Apr 20, 2023

jtremback Apr 20, 2023

jtremback Apr 20, 2023

jtremback Apr 20, 2023

jtremback Apr 20, 2023

faddat commented Apr 21, 2023

mpoke commented Apr 21, 2023 •

edited

Loading

faddat commented Apr 25, 2023

		@@ -3,23 +3,3 @@ sidebar_position: 2
		---

		# Consumer Chain Governance

New branch sdk47 #817

New branch sdk47 #817

Conversation

sontrinh16 commented Apr 2, 2023 • edited Loading

Description

Type of change

Versioning Implications

Targeting

faddat commented Apr 20, 2023

faddat commented Apr 20, 2023 • edited Loading

mpoke commented Apr 20, 2023

tac0turtle commented Apr 20, 2023

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

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

faddat commented Apr 21, 2023

mpoke commented Apr 21, 2023 • edited Loading

faddat commented Apr 25, 2023

sontrinh16 commented Apr 2, 2023 •

edited

Loading

faddat commented Apr 20, 2023 •

edited

Loading

mpoke commented Apr 21, 2023 •

edited

Loading