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

chore(ibc): migrate IBC transfer escrow accounts to module account type #1156

Merged
merged 19 commits into from Dec 15, 2022

Conversation

GAtom22
Copy link
Contributor

@GAtom22 GAtom22 commented Dec 8, 2022

Description

Migrate the IBC transfer escrow accounts to the ModuleAccount type

Closes: https://linear.app/evmos/issue/ENG-968/migrate-ibc-transfer-escrow-accounts-to-moduleaccounts-pr-to-ibc-go


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.

PR review checkboxes:

I have...

  • added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • included the correct type prefix in the PR title
  • targeted the correct branch (see PR Targeting)
  • provided a link in the PR description to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all required CI checks have passed

Code maintenance:

I have...

  • written unit and integration tests
  • added relevant godoc and code comments.
  • updated relevant documentation (docs/) or specification (x/<module>/spec/)

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 all author checklist items have been addressed
  • confirmed that this PR does not change production code

@GAtom22 GAtom22 added the types label Dec 8, 2022
@codecov
Copy link

codecov bot commented Dec 8, 2022

Codecov Report

Merging #1156 (936e270) into main (c46878a) will increase coverage by 0.16%.
The diff coverage is 84.61%.

❗ Current head 936e270 differs from pull request most recent head 089676b. Consider uploading reports for the commit 089676b to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1156      +/-   ##
==========================================
+ Coverage   78.43%   78.59%   +0.16%     
==========================================
  Files         130      132       +2     
  Lines        7767     7802      +35     
==========================================
+ Hits         6092     6132      +40     
+ Misses       1524     1521       -3     
+ Partials      151      149       -2     
Impacted Files Coverage Δ
x/ibc/transfer/keeper/migrations.go 66.66% <66.66%> (ø)
x/ibc/transfer/migrations/v3/migration.go 90.00% <90.00%> (ø)
x/vesting/keeper/msg_server.go 89.16% <0.00%> (+4.31%) ⬆️

@GAtom22 GAtom22 changed the title chore: migrate ibc transfer escrow accounts to module account type chore(ibc): migrate IBC transfer escrow accounts to module account type Dec 8, 2022
@linear
Copy link

linear bot commented Dec 8, 2022

ENG-968 migrate ibc transfer escrow accounts to ModuleAccounts (PR to ibc-go)

Current

No ibc module account is set on maccPerms setup step during application setup, so that at chain genesis ibc was not registered as a module-account.

	// module account permissions
	maccPerms = map[string][]string{
		authtypes.FeeCollectorName:     nil,
		distrtypes.ModuleName:          nil,
		stakingtypes.BondedPoolName:    {authtypes.Burner, authtypes.Staking},
		stakingtypes.NotBondedPoolName: {authtypes.Burner, authtypes.Staking},
		govtypes.ModuleName:            {authtypes.Burner},
		ibctransfertypes.ModuleName:    {authtypes.Minter, authtypes.Burner},
		evmtypes.ModuleName:            {authtypes.Minter, authtypes.Burner}, // used for secure addition and subtraction of balance using module account
		inflationtypes.ModuleName:      {authtypes.Minter},
		erc20types.ModuleName:          {authtypes.Minter, authtypes.Burner},
		claimstypes.ModuleName:         nil,
		incentivestypes.ModuleName:     {authtypes.Minter, authtypes.Burner},
	}

Instead, escrowed tokens for ibc transfers are now escrowed on a EOA account: https://www.mintscan.io/evmos/account/evmos1a53udazy8ayufvy0s434pfwjcedzqv345dnt3x

After manual addition:

. . .
:         {authtypes.Minter, authtypes.Burner},
. . .

we found that querying auth module-account yields address:

evmos1xf3u97sfqwf069q7zvrsahxluash58pavpmg7q

which is differs from actual one that is used on mainnet.

Expected

Registered ibc module account with corresponding address:

Notes

  • add in post-mortem
  • Do we need the ibchost.ModuleName or ibctransfer.ModuleName ?

@GAtom22 GAtom22 marked this pull request as ready for review December 8, 2022 21:14
@GAtom22 GAtom22 requested a review from a team as a code owner December 8, 2022 21:14
@GAtom22 GAtom22 requested review from hanchon, 0a1c, fedekunze, danburck, 4rgon4ut, adisaran64, facs95, MalteHerrmann and Vvaradinov and removed request for a team December 8, 2022 21:14
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Thanks @GAtom22, great work! However, I we shouldn't use the module Migration route, but we should instead run this logic as part of the UpgradeHandler logic. The reason why we should do this is to avoid having different versions and logic between the canonical transfer module from ibc-go and our wrapper module

x/ibc/transfer/migrations/v3/migration.go Outdated Show resolved Hide resolved
x/ibc/transfer/migrations/v3/migration.go Outdated Show resolved Hide resolved
Copy link
Contributor

@4rgon4ut 4rgon4ut left a comment

Choose a reason for hiding this comment

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

@GAtom22 God job man! Suggested couple of little fixes down here.

x/ibc/transfer/keeper/migrations.go Outdated Show resolved Hide resolved
x/ibc/transfer/module.go Outdated Show resolved Hide resolved
Co-authored-by: 4rgon4ut <59182467+4rgon4ut@users.noreply.github.com>
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
@github-actions github-actions bot removed the types label Dec 9, 2022
@GAtom22
Copy link
Contributor Author

GAtom22 commented Dec 10, 2022

Thanks @GAtom22, great work! However, I we shouldn't use the module Migration route, but we should instead run this logic as part of the UpgradeHandler logic. The reason why we should do this is to avoid having different versions and logic between the canonical transfer module from ibc-go and our wrapper module

@fedekunze, should I add it to the v10.0.0 UpgradeHandler or should I leave it on the v10.1.0?

Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

@GAtom22 much better now. Can you test that the Module Account is valid? otherwise the genesis validation will fail

app/upgrades/v10_1/upgrades_test.go Outdated Show resolved Hide resolved
@fedekunze
Copy link
Contributor

should I add it to the v10.0.0 UpgradeHandler or should I leave it on the v10.1.0?

we can create a new file for v11

GAtom22 and others added 2 commits December 13, 2022 09:20
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM. Could you delete the 2 extra files?

delegations_1.txt Outdated Show resolved Hide resolved
validators_out.txt Outdated Show resolved Hide resolved
@fedekunze fedekunze enabled auto-merge (squash) December 14, 2022 23:06
@fedekunze fedekunze merged commit b442f1e into main Dec 15, 2022
@fedekunze fedekunze deleted the GAtom22/migrate-ibc-escrow-acc branch December 15, 2022 14:13
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.

None yet

3 participants