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: Bridge module and upgrade sdk50 #179

Merged
merged 55 commits into from
Nov 15, 2024

Conversation

trinitys7
Copy link
Contributor

No description provided.

@trinitys7 trinitys7 requested a review from jiujiteiro as a code owner October 4, 2024 16:36
@trinitys7 trinitys7 changed the title feat: Upgrade sdk50 feat: Bridge module and upgrade sdk50 Oct 18, 2024
@jiujiteiro jiujiteiro requested a review from facs95 October 18, 2024 15:06
app/app.go Dismissed Show dismissed Hide dismissed
app/forks.go Dismissed Show dismissed Hide dismissed
valIter := app.StakingKeeper.ValidatorQueueIterator(ctx, oneEnternityLater, 99999999999999)
valIter, err := app.StakingKeeper.ValidatorQueueIterator(ctx, oneEnternityLater, 99999999999999)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods Warning

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
unbondingTimesliceIterator := sk.UBDQueueIterator(ctx, oneEnternityLater) // make sure to iterate all queue
unbondingTimesliceIterator, err := sk.UBDQueueIterator(ctx, oneEnternityLater) // make sure to iterate all queue
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods Warning

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
@neitdung
Copy link
Collaborator

neitdung commented Oct 30, 2024

@trinitys7 @catShaark do we need to write a migrate function for converting KVStore to collections

@trinitys7
Copy link
Contributor Author

@trinitys7 @catShaark do we need to write a migrate function for converting KVStore to collections

No we don't, I tested them on mainnet state upgrade

app/ante/ante.go Outdated Show resolved Hide resolved
app/ante/ante.go Show resolved Hide resolved
app/app.go Outdated Show resolved Hide resolved
app/app.go Outdated Show resolved Hide resolved
// Ethereum keys
meter.ConsumeGas(evmosante.Secp256k1VerifyCost, "ante verify: eth_secp256k1")
return nil
case *ethsecp256k1.PubKey:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For backward compability with acount that has ethermint key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ethsecp256k1 Pubkey is the pubkey for ethermint one, while the osecp256k1 Pubkey is the pubkey for evmos os, as we move from ethermint to os, the ethermint key will still be store in local keyring.

Copy link
Collaborator

Choose a reason for hiding this comment

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

you are right, I can ask the evmos team if they can support the ethermint type so we dont have to support mulutiple types

app/app.go Show resolved Hide resolved
app/encoding.go Show resolved Hide resolved
@facs95
Copy link
Collaborator

facs95 commented Nov 5, 2024

PR should be block until fixing the ante handler issues. I stopped the review there giving that that change should have significant changes in multiple files. Once it is fixed I will continue the review.

app/upgrades/v2/upgrades.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@jiujiteiro jiujiteiro left a comment

Choose a reason for hiding this comment

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

lgtm

@jiujiteiro jiujiteiro merged commit fd75b67 into realiotech:main Nov 15, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants