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

Handle duplicate members in cw4-group create #702

Merged
merged 9 commits into from Oct 13, 2022
Merged

Conversation

codehans
Copy link
Contributor

When a cw4-group contract is instantiated with a list of members where some are duplicated, the iterator overwrites any previous member weight, whilst still incrementing the total weight. This PR merges duplicate weights in the create function

@CLAassistant
Copy link

CLAassistant commented Jul 8, 2022

CLA assistant check
All committers have signed the CLA.

@ueco-jb
Copy link
Member

ueco-jb commented Jul 8, 2022

@codehans will you pick up that PR and add some proper test?

@0xekez
Copy link
Contributor

0xekez commented Sep 14, 2022

fwiw - i do think this could be considered to be a non-backwards compatible change.

for example, before our audit (see the first issue there) we did not dedup the members list when instantiating a dao that uses cw4-groups. we now do, but were this to land we'd probably stop doing that to be consistent with the new behavior.

@codehans
Copy link
Contributor Author

apologies @ueco-jb @ezekiiel missed this one. this isn't something we're actively using or particularly precious about. happy to leave as-is, especially as time is fairly scarce these days

@uint
Copy link
Contributor

uint commented Oct 12, 2022

This looks like a serious flaw. If you don't mind, I'll pick this up, rebase, add a test, and merge. @codehans do you mind if I push here?

@uint
Copy link
Contributor

uint commented Oct 12, 2022

fwiw - i do think this could be considered to be a non-backwards compatible change.

@0xekez That's fair! I don't think we guarantee backwards-compatibility currently (0.* semantic versions). Is this going to cause problems for you?

@codehans
Copy link
Contributor Author

This looks like a serious flaw. If you don't mind, I'll pick this up, rebase, add a test, and merge. @codehans do you mind if I push here?

go ahead!

@uint
Copy link
Contributor

uint commented Oct 12, 2022

@codehans Would you mind signing the CLA thing? I'll need that to merge later.

@uint
Copy link
Contributor

uint commented Oct 12, 2022

Actually, I'm going to have duplicate entries throw an error since it's not clear what outcome the user expected. As things stand, this PR would cause instantiation and ExecuteMsg::UpdateMembers to have inconsistent behavior.

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

lgtm with error on duplicates

@uint uint merged commit 96c5927 into CosmWasm:main Oct 13, 2022
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

6 participants