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.

Sign up for GitHub

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

Jump to bottom

feat : upgraded the app with consensus module to SDK v0.50.0 #1038

Merged
merged 3 commits into from
Dec 15, 2023
Merged

feat : upgraded the app with consensus module to SDK v0.50.0 #1038

merged 3 commits into from
Dec 15, 2023

Conversation

ruthishvitwit
Copy link

No description provided.

@ruthishvitwit ruthishvitwit marked this pull request as ready for review November 24, 2023 05:41
byte-bandit
byte-bandit requested changes Nov 27, 2023
View reviewed changes
x/evm/module.go Outdated Show resolved Hide resolved
x/evm/keeper/smart_contract_deployment.go Outdated Show resolved Hide resolved
x/evm/keeper/smart_contract_deployment.go Outdated Show resolved Hide resolved
@aleem1314
Copy link
Contributor

Check #1036 PR review comments. Apply relevant changes here too.

@byte-bandit
Copy link
Contributor

Also #1044

aleem1314
aleem1314 suggested changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

@aleem1314 aleem1314 left a comment

Choose a reason for hiding this comment

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

Still there are few things need to be updated.
Check #1036 (review)

aleem1314
aleem1314 reviewed Dec 8, 2023
View reviewed changes
x/valset/module.go Outdated Show resolved Hide resolved
aleem1314
aleem1314 reviewed Dec 8, 2023
View reviewed changes
x/evm/keeper/attest.go Outdated Show resolved Hide resolved
aleem1314
aleem1314 reviewed Dec 8, 2023
View reviewed changes
x/consensus/module.go Outdated Show resolved Hide resolved
byte-bandit
byte-bandit requested changes Dec 8, 2023
View reviewed changes
x/evm/keeper/attest.go Outdated Show resolved Hide resolved
x/evm/keeper/attest.go Outdated Show resolved Hide resolved
x/evm/keeper/smart_contract_deployment.go Outdated Show resolved Hide resolved
x/gravity/keeper/attestation.go Outdated Show resolved Hide resolved
byte-bandit
byte-bandit requested changes Dec 11, 2023
View reviewed changes
x/consensus/keeper/consensus/consensus_test.go Outdated Show resolved Hide resolved
x/consensus/keeper/keeper.go Outdated Show resolved Hide resolved
x/consensus/types/message_add_evidence.go Outdated Show resolved Hide resolved
byte-bandit
byte-bandit reviewed Dec 11, 2023
View reviewed changes
Copy link
Contributor

@byte-bandit byte-bandit left a comment

Choose a reason for hiding this comment

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

Only one small thing from my end.

x/consensus/keeper/concensus_keeper.go Outdated Show resolved Hide resolved
x/consensus/keeper/concensus_keeper.go Outdated Show resolved Hide resolved
@ruthishvitwit ruthishvitwit changed the title feat : upgraded the app with consensus module to SDK v0.5 feat : upgraded the app with consensus module to SDK v0.50.0 Dec 12, 2023
byte-bandit
byte-bandit previously approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor

@byte-bandit byte-bandit left a comment

Choose a reason for hiding this comment

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

lgtm, final review to @aleem1314

aleem1314
aleem1314 previously approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@aleem1314 aleem1314 left a comment

Choose a reason for hiding this comment

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

LGTM

@aleem1314 aleem1314 enabled auto-merge (squash) December 13, 2023 03:42
@deepan95dev
Copy link
Contributor

deepan95dev commented Dec 13, 2023 •
edited
Loading

@byte-bandit if everything looks good in this module, lets merge with v0.50.0-upgrade branch. so that we will update other modules accordingly which uses consensus module.

@taariq taariq removed the request for review from alexanderbez December 13, 2023 08:09
@taariq
Copy link
Contributor

taariq commented Dec 13, 2023

@byte-bandit do we not want checks to be passing before we merge?

@byte-bandit
Copy link
Contributor

@taariq I don't think that's realistic. Changing each module separately is going to break all tests and CI. It's not great, but not horrible. We definitely want a full green CI pipeline before merging back to master though, with updated and expanded test coverage.

@byte-bandit
Copy link
Contributor

@deepan95dev I disabled the need for green CI for now, but we need a cleaner commit history.

I suggest to go over the the 50 pending commits, rebase and fixup where useful. Please use conventional commits. We require signatures as well: https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

Once that's done, feel free to mege.

@byte-bandit
Copy link
Contributor

@deepan95dev Actually, I enabled the linter again. There's no reason why we should not keep it green. Please make sure it passes as well.

@aleem1314
Copy link
Contributor

@deepan95dev Actually, I enabled the linter again. There's no reason why we should not keep it green. Please make sure it passes as well.

Hey @byte-bandit , We need to fix the go.mod file before running the linter. After updating all modules, we'll create separate pull requests to address test and lint issues.

@byte-bandit
Copy link
Contributor

@aleem1314 Is there a way we can keep the go.mod alive for these PRs? We can reduce the noise of auto generated files and whitespace changes by enforcing linter usage.

@aleem1314
Copy link
Contributor

aleem1314 commented Dec 13, 2023 •
edited
Loading

@aleem1314 Is there a way we can keep the go.mod alive for these PRs? We can reduce the noise of auto generated files and whitespace changes by enforcing linter usage.

I don't think it is possible, because few modules are pointing to SDK v0.47.x and few modules pointing to v0.50.x. go mod updates always failing because of this.

@byte-bandit
Copy link
Contributor

@aleem1314 Is there a way we can keep the go.mod alive for these PRs? We can reduce the noise of auto generated files and whitespace changes by enforcing linter usage.

I don't think it is possible, because few modules are pointing to SDK v0.47.x and few modules pointing to v0.50.x. go mod updates always failing because of this.

Okay, let's keep it rolling. Need for linters removed for v0.50.0-upgrade branch.

@ruthishvitwit ruthishvitwit dismissed stale reviews from aleem1314 and byte-bandit via 5d73fa2 December 13, 2023 13:46
auto-merge was automatically disabled December 13, 2023 17:33

Pull request was closed

@deepan95dev deepan95dev reopened this Dec 14, 2023
@aleem1314 aleem1314 enabled auto-merge (squash) December 14, 2023 05:22
@deepan95dev
Copy link
Contributor

@byte-bandit rebase has been done. Please approve to merge this PR

taariq
taariq approved these changes Dec 15, 2023
View reviewed changes
Copy link
Contributor

@taariq taariq left a comment

Choose a reason for hiding this comment

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

LGTM!

@taariq
Copy link
Contributor

taariq commented Dec 15, 2023

@aleem1314 Need your final review here as well to merge. Thanks!

@aleem1314 aleem1314 merged commit 1721649 into v0.50.0-upgrade Dec 15, 2023
0 of 2 checks passed
@aleem1314 aleem1314 deleted the consensus-changes branch December 15, 2023 08:45
@byte-bandit byte-bandit mentioned this pull request Jan 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aleem1314 aleem1314 aleem1314 approved these changes

@taariq taariq taariq approved these changes

@byte-bandit byte-bandit byte-bandit left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ruthishvitwit @aleem1314 @byte-bandit @deepan95dev @taariq