Mutable Metadata group name Part 1 #643
Conversation
cameronvoell
force-pushed
the
cv/mutable-metadata-group-name-part-1
branch
from
a4e45ca to
052dc43
Compare
| } | ||
| } | ||
|
|
||
| pub(crate) fn from_proto( |
There was a problem hiding this comment.
we have both from/to proto and re-use in the trait
There was a problem hiding this comment.
I definitely copied from xmtp_mls/src/groups/group_metadata.rs, but I agree with your thinking (if I'm understanding correctly).
Updated here => d91fe5b Let me know if you meant something else.
xmtp_mls/src/groups/intents.rs
Outdated
| Self { group_name } | ||
| } | ||
|
|
||
| pub(crate) fn to_bytes(&self) -> Vec<u8> { |
There was a problem hiding this comment.
Should we just unify all the conversion methods in the TryFrom to avoid confusion; what's the benefit of having the method on UpdateMetadataIntentData?
There was a problem hiding this comment.
Updated here. I think this is what you were suggesting, but let me know if you meant something else.
| @@ -231,19 +231,22 @@ pub struct PolicySet { | |||
| pub remove_member_policy: MembershipPolicies, | |||
| pub add_installation_policy: MembershipPolicies, | |||
| pub remove_installation_policy: MembershipPolicies, | |||
| pub update_group_name_policy: MembershipPolicies, | |||
There was a problem hiding this comment.
I kinda think we should have a new type of policy for metadata. Some of the MembershipPolicies aren't applicable here (for example, AllowSameMember) and it makes easier to be misused having that as an option.
What if we had a different type, like MetadataPolicy that can be more tailored towards updating metadata.
There was a problem hiding this comment.
Good point, ill make that update 👍
There was a problem hiding this comment.
@neekolas updating to include a new MetadataPolicy type actually adds a lot of cascading updates, since it touches all the validation logic. I was going to tackle validation logic in part 3, so how do we feel about leaving MetadataPolicy to the part 3 PR?
cameronvoell
force-pushed
the
cv/mutable-metadata-group-name-part-1
branch
from
eb1143e to
a236ed4
Compare
Thanks for the feedback @insipx . I liked you suggestion to put all conversion logic in TryFrom functions and avoiding having redundant functions. I created an issue here => #657 for cleaning up other instances of this pattern, because I think there are quite a few, but it is probably best to keep outside the scope of this PR. Let me know if you think I misunderstood your suggestion 🙇 |
Part 1 for #548
We would like to have a metadata extension type that can be modified after group creation. This is possible using the Unknown Group Context Extension type, which accepts a byte array as the only field. See #548 for more info.
Related PR's:
Before merging this to
main:Cargo.tomlto commit onxmtp/openmlsmainbranchxmtp-protorepomainbranch