fix(group): migrate group policy account to base accounts with credentials #13742

julienrbrt · 2022-11-03T10:12:36Z

Description

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...

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)

server/start.go

x/group/keeper/msg_server.go

codecov · 2022-11-03T12:26:48Z

Codecov Report

Merging #13742 (0a2c2d3) into main (c7d4921) will increase coverage by 0.11%.
The diff coverage is 57.98%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13742      +/-   ##
==========================================
+ Coverage   56.26%   56.37%   +0.11%     
==========================================
  Files         664      676      +12     
  Lines       56484    56989     +505     
==========================================
+ Hits        31780    32129     +349     
- Misses      22130    22243     +113     
- Partials     2574     2617      +43

Impacted Files	Coverage Δ
server/mock/tx.go	`53.33% <0.00%> (ø)`
x/group/keeper/migrations.go	`0.00% <0.00%> (ø)`
x/group/module/module.go	`48.14% <40.00%> (+0.09%)`	⬆️
x/auth/types/credentials.go	`62.50% <62.50%> (ø)`
x/group/keeper/msg_server.go	`68.26% <66.66%> (-0.49%)`	⬇️
x/group/migrations/v2/migrate.go	`72.72% <72.72%> (ø)`
x/auth/types/codec.go	`100.00% <100.00%> (ø)`
tx/textual/valuerenderer/int.go	`50.00% <0.00%> (ø)`
tx/textual/valuerenderer/bytes.go	`62.50% <0.00%> (ø)`
tx/textual/valuerenderer/valuerenderer.go	`72.41% <0.00%> (ø)`
... and 7 more

x/group/keeper/msg_server.go

…, `v{Consensus}` (#13772) * chore: rename migrations from v42, v43 to v1, v2 * group migration should be in #13742 * updates

x/group/migrations/v2/migrate.go

amaury1093

I'm not sure this is my preferred direction for this PR.

My preference:

Do the most correct way, i.e convert from ModuleAccount to GroupPolicyInfo
A totally new PR, which simply removes these 3 lines. They are anyways wrong. And it solves group: validate genesis failing after creating group policy account #13445.
Convert from ModuleAccount to BaseAccount, i.e. this PR.

If our immediate goal is to fix #13445, and 1 might delay v0.47 release, then I would actually prefer 2 over 3.

x/group/keeper/msg_server.go

julienrbrt · 2022-11-10T18:48:20Z

I'm not sure this is my preferred direction for this PR.

My preference:

Do the most correct way, i.e convert from ModuleAccount to GroupPolicyInfo

A totally new PR, which simply removes these 3 lines. They are anyways wrong. And it solves group: validate genesis failing after creating group policy account #13445.

Convert from ModuleAccount to BaseAccount, i.e. this PR.

If our immediate goal is to fix #13445, and 1 might delay v0.47 release, then I would actually prefer 2 over 3.

As discussed on Slack, I'll continue with 3, but set an unclaimable pubkey to the account.

crypto/codec/proto.go

x/group/types.go

aaronc · 2022-11-11T02:11:11Z

If we're going to the trouble of creating this unclaimable pubkey, why can't we just create something specific for groups, i.e. GroupAccountCredential as outlined in #13211? Since we're going to this much effort I'd rather move towards the right solution

…tials

x/auth/types/credentials.go

likhita-809

nice work!

aaronc

Good work @julienrbrt. Generally LGTM. Left a few small comments for things I think should be addressed

proto/cosmos/auth/v1beta1/auth.proto

x/auth/types/credentials.go

aaronc · 2022-11-21T19:25:34Z

x/auth/types/credentials.go

+	return false
+}
+
+func (m *ModuleCredential) Type() string {


Unrelated to this PR, but is this method on PubKey even used anymore? Maybe we should get rid of it

After a brief look, it is used for comparison (easy to remove) and in keyring

cosmos-sdk/crypto/keyring/keyring.go

Line 270 in 4882f93

return crypto.ArmorPubKeyBytes(bz, key.Type()), nil

.

x/auth/types/credentials.go

x/group/migrations/v2/migrate_test.go

x/auth/types/credentials.go

sonarcloud · 2022-11-21T22:40:11Z

[Cosmos SDK] SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

66.3% Coverage
0.0% Duplication

…tials (#13742) (cherry picked from commit d6da703)

robert-zaremba · 2022-11-21T23:36:18Z

x/auth/types/credentials.go

+
+	for i := range m.DerivationKeys {
+		for j := range m.DerivationKeys[i] {
+			if m.DerivationKeys[i][j] != om.DerivationKeys[i][j] {


you should use bytes.Compare here

…tials (backport #13742) (#13962) * fix(group): migrate group policy account to base accounts with credentials (#13742) (cherry picked from commit d6da703) * update changelog * updates Co-authored-by: Julien Robert <julien@rbrt.fr>

github-actions bot added C:x/group Type: Build labels Nov 3, 2022

julienrbrt changed the title ~~fix: set group policy name properly~~ fix: do not set group policy account name Nov 3, 2022

julienrbrt mentioned this pull request Nov 3, 2022

fix: nil pointer when quitting simd start #13743

Merged

19 tasks

github-advanced-security bot found potential problems Nov 3, 2022

View reviewed changes

server/start.go Fixed Show fixed Hide fixed

julienrbrt commented Nov 3, 2022

View reviewed changes

x/group/keeper/msg_server.go Show resolved Hide resolved

github-actions bot removed the Type: Build label Nov 3, 2022

amaury1093 reviewed Nov 3, 2022

View reviewed changes

x/group/keeper/msg_server.go Show resolved Hide resolved

x/group/keeper/msg_server.go Outdated Show resolved Hide resolved

amaury1093 reviewed Nov 3, 2022

View reviewed changes

x/group/keeper/msg_server.go Show resolved Hide resolved

julienrbrt mentioned this pull request Nov 4, 2022

refactor: rename migrations from v42, v43, v{SDK} to v1, v2, v{Consensus} #13772

Merged

19 tasks

julienrbrt added a commit that referenced this pull request Nov 4, 2022

group migration should be in #13742

04ed453

julienrbrt added a commit that referenced this pull request Nov 7, 2022

refactor: rename migrations from v42, v43, v{SDK} to v1, v2…

7efeb82

…, `v{Consensus}` (#13772) * chore: rename migrations from v42, v43 to v1, v2 * group migration should be in #13742 * updates

github-advanced-security bot found potential problems Nov 7, 2022

View reviewed changes

x/group/migrations/v2/migrate.go Fixed Show fixed Hide fixed

julienrbrt marked this pull request as ready for review November 10, 2022 15:06

julienrbrt requested a review from a team as a code owner November 10, 2022 15:06

github-actions bot added C:x/crisis C:x/distribution distribution module related C:x/slashing C:x/staking labels Nov 10, 2022

tac0turtle assigned amaury1093 and atheeshp Nov 10, 2022

julienrbrt added the T: State Machine Breaking State machine breaking changes (impacts consensus). label Nov 10, 2022

amaury1093 reviewed Nov 10, 2022

View reviewed changes

x/group/keeper/msg_server.go Outdated Show resolved Hide resolved

julienrbrt commented Nov 10, 2022

View reviewed changes

crypto/codec/proto.go Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Nov 10, 2022

View reviewed changes

x/group/types.go Fixed Show fixed Hide fixed

julienrbrt mentioned this pull request Nov 11, 2022

fix: .String() panics when pubkey is set on a BaseAccount #13838

Merged

19 tasks

julienrbrt added the backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release label Nov 11, 2022

github-actions bot removed C:Rosetta Issues and PR related to Rosetta C:CLI labels Nov 17, 2022

fix(group): Migrate group policy account to base accounts with creden…

97930f7

…tials

julienrbrt force-pushed the julien/group-policy branch from 9960abc to 97930f7 Compare November 17, 2022 19:42

julienrbrt changed the title ~~fix: do not set group policy account name~~ fix(group): migrate group policy account to base accounts with credentials Nov 17, 2022

alexanderbez reviewed Nov 18, 2022

View reviewed changes

x/auth/types/credentials.go Outdated Show resolved Hide resolved

likhita-809 approved these changes Nov 18, 2022

View reviewed changes

julienrbrt requested a review from aaronc November 18, 2022 15:12

julienrbrt added 2 commits November 18, 2022 17:51

Merge branch 'main' into julien/group-policy

b4c5c79

Merge branch 'main' into julien/group-policy

3569eaf

aaronc reviewed Nov 21, 2022

View reviewed changes

julienrbrt mentioned this pull request Nov 21, 2022

Add Genesis JSON migration for group #13959

Closed

julienrbrt added 2 commits November 21, 2022 20:58

implement feedback

285278e

add more addresses

7dcfa4a

julienrbrt requested a review from aaronc November 21, 2022 20:13

Merge branch 'main' into julien/group-policy

ddc4839

aaronc reviewed Nov 21, 2022

View reviewed changes

x/auth/types/credentials.go Outdated Show resolved Hide resolved

aaronc approved these changes Nov 21, 2022

View reviewed changes

updates

83bfead

julienrbrt enabled auto-merge (squash) November 21, 2022 22:19

updates

0a2c2d3

julienrbrt merged commit d6da703 into main Nov 21, 2022

julienrbrt deleted the julien/group-policy branch November 21, 2022 22:46

mergify bot mentioned this pull request Nov 21, 2022

fix(group): migrate group policy account to base accounts with credentials (backport #13742) #13962

Merged

mergify bot pushed a commit that referenced this pull request Nov 21, 2022

fix(group): migrate group policy account to base accounts with creden…

4f70085

…tials (#13742) (cherry picked from commit d6da703)

robert-zaremba reviewed Nov 21, 2022

View reviewed changes

aaronc mentioned this pull request Jan 7, 2023

Design issue: Bank sent to a fresh address should not initialize a base account #14453

Closed

0xstepit mentioned this pull request Jul 1, 2024

chore(cachemulti): add Copy method evmos/cosmos-sdk#52

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(group): migrate group policy account to base accounts with credentials #13742

fix(group): migrate group policy account to base accounts with credentials #13742

julienrbrt commented Nov 3, 2022

codecov bot commented Nov 3, 2022 •

edited

Loading

amaury1093 left a comment •

edited

Loading

julienrbrt commented Nov 10, 2022

aaronc commented Nov 11, 2022

likhita-809 left a comment

aaronc left a comment

aaronc Nov 21, 2022

julienrbrt Nov 21, 2022

sonarcloud bot commented Nov 21, 2022

robert-zaremba Nov 21, 2022

fix(group): migrate group policy account to base accounts with credentials #13742

fix(group): migrate group policy account to base accounts with credentials #13742

Conversation

julienrbrt commented Nov 3, 2022

Description

Author Checklist

Reviewers Checklist

codecov bot commented Nov 3, 2022 • edited Loading

Codecov Report

amaury1093 left a comment • edited Loading

Choose a reason for hiding this comment

julienrbrt commented Nov 10, 2022

aaronc commented Nov 11, 2022

likhita-809 left a comment

Choose a reason for hiding this comment

aaronc left a comment

Choose a reason for hiding this comment

aaronc Nov 21, 2022

Choose a reason for hiding this comment

julienrbrt Nov 21, 2022

Choose a reason for hiding this comment

sonarcloud bot commented Nov 21, 2022

robert-zaremba Nov 21, 2022

Choose a reason for hiding this comment

codecov bot commented Nov 3, 2022 •

edited

Loading

amaury1093 left a comment •

edited

Loading