Who Added Me #614
Who Added Me #614
Conversation
zombieobject
force-pushed
the
em/staged-welcome
branch
2 times, most recently
from
5c76b40
to
d0c98df
Compare
zombieobject
force-pushed
the
em/staged-welcome
branch
2 times, most recently
from
43f6bbc
to
4b613a3
Compare
zombieobject
requested review from
richardhuaaa,
neekolas,
tuddman and
cameronvoell
zombieobject
force-pushed
the
em/staged-welcome
branch
from
6aab4f6
to
2c1ee21
Compare
| client, | ||
| group_id, | ||
| stored_group.created_at_ns, | ||
| added_by_address, |
There was a problem hiding this comment.
Ethan and I paired and got on the same page. My initial hesitation was that I didn't understand why we needed to set the address on the MlsGroup::new and also on the StoredGroup:new seems like we should just be able to set it on the stored group and then fetch it from the stored group? Came to the conclusion that we don't do anything with the storedGroup in the bindings so this seems like the best way given our current architecture cc @richardhuaaa if I'm missing some context.
There was a problem hiding this comment.
@nplasterer it's a good point. Today, we're fetching the StoredGroup, and then manually caching the values (created_at_ns, added_by_address) onto the MlsGroup via the constructor so that we don't need to re-fetch it again. The drawback is that the constructor for MlsGroup is getting more complicated over time. Could be refactored one of two ways:
-
Don't cache the values onto the MlsGroup. This is simpler code, but tradeoff is you need to re-fetch from the DB every time you need a property like
added_by. -
Instead of manually copying the values over to the MlsGroup, have the MlsGroup hold a copy of StoredGroup inside it so there's no duplication. That's probably the best of both worlds.
There was a problem hiding this comment.
I like 2. do you think this will be hard to do later/a breaking change? Just seeing how we should prioritize that work. My gut is saying we should probably do it now before we filter this through the sdks.
zombieobject
force-pushed
the
em/staged-welcome
branch
from
7e975f4
to
4de1ff8
Compare
|
@nplasterer The requested reverted changes are now complete and ready for review. |
Set FfiGroupMember on FfiGroup Clean up formatting
zombieobject
force-pushed
the
em/staged-welcome
branch
4 times, most recently
from
a51b06e
to
ccfc9ee
Compare
zombieobject
force-pushed
the
em/staged-welcome
branch
from
ccfc9ee
to
3fc71ca
Compare
|
I am passing on this flaky failing test as originally suggested by @richardhuaaa. It passes locally without fail. I have on occasion had it pass in CI. That said, it's a bad test that needs to be refactored. With that out of the way, this PR is ready to go. |
nplasterer
dismissed
their stale review
This looks good from my perspective but would like someone else to review it since we paired.
bindings_ffi/src/mls.rs
Outdated
| let out = Arc::new(FfiGroup { | ||
| inner_client: self.inner_client.clone(), | ||
| group_id: convo.group_id, | ||
| created_at_ns: convo.created_at_ns, | ||
| added_by_address: Some(self.inner_client.account_address()), |
There was a problem hiding this comment.
We should read this from convo so that the value is guaranteed to be consistent with convo
There was a problem hiding this comment.
Good catch. Thank you!
I think that makes sense. Better to break things now than later. 👍 |
| @@ -0,0 +1,2 @@ | |||
| ALTER TABLE groups | |||
| ADD COLUMN added_by_address TEXT | |||
There was a problem hiding this comment.
@zombieobject I think @richardhuaaa was suggesting that this is NOT NULL do you mind doing that in a follow up PR?
There was a problem hiding this comment.
@nplasterer I missed that comment just as I was merging the PR. I am more than happy to create a follow up PR to address this request.
For context, there were 2 reasons for the optionality of that parameter. The largest one IMHO was due to MLSGroup lifecycle, being that the address of "who added me" was not always available at group creation. The latter was breaking changes, but as you both stated, better to break things now, to which I concur.
I'll open up a follow up PR with the request immediately and see what I can do to address the lifecycle questions. 🙂
Save the wallet address of who added a user to a group.