feat: Pagination on Consent Load #307
Conversation
|
Just using for verifying a case |
|
We could make the 2k wallet on the dev network have a bunch of consent records to test this as well? |
We can, was working with Daria and she had a similar wallet to the one seeing a problem and it didn't seem specific to that use case. However it could be ideal to make the 2k wallet have additional consent records for testing this and verifying performance in the future |
alexrisch
force-pushed
the
ar/consent-test
branch
from
e32fbe7 to
44123b9
Compare
|
I've added a couple thousand consent entries on to the dev wallet, this will make testing a little more real in the future |
Added pagination to consent refresh call
alexrisch
force-pushed
the
ar/consent-test
branch
from
44123b9 to
9efc8eb
Compare
alexrisch
changed the title
| guard let identifier = identifier else { | ||
| throw ContactError.invalidIdentifier | ||
| } | ||
|
|
||
| let envelopes = try await client.apiClient.envelopes(topic: Topic.preferenceList(identifier).description, pagination: Pagination(direction: .ascending)) | ||
| let pagination = pagination ?? Pagination(direction: .ascending) | ||
| let envelopes = try await client.apiClient.envelopes(topic: Topic.preferenceList(identifier).description, pagination: pagination) |
There was a problem hiding this comment.
Isn't this already paginated under the hood?
What problem are we solving by putting the user in control of pagination here? When would they want to do anything other than load the entire consent list? Anything less would leave them in an out of date state.
There was a problem hiding this comment.
Updated to only take an after date to allow for more performant local cache updates
There was a problem hiding this comment.
I think there needs to be a bigger change here because the refreshConsentList just wipes out the current list and makes a new list. I think the ideal scenario is that the consentList doesn't get reset. On every load and that you could just fetch the latest consent items from the last time that you fetched consent. But I think you'll need more changes for that.
There was a problem hiding this comment.
This is kinda my issue with current consent, I think it's nuanced (at least personally)
There was a problem hiding this comment.
An integrator would only use afterDate if they were caching the consent list, so they likely would not be using the SDK memory cached consent list after refreshing
There was a problem hiding this comment.
An integrator would only use afterDate if they were caching the consent list
Ahh. I see. But our SDK also needs an up-to-date consent list, doesn't it? Is the idea that the integrator would pass their known entries back in to us from their cache on startup?
Alternatively, we could just handle caching for the integrator. I know on iOS there's all kinds of permissions implications to handling persistence, but maybe it's worthwhile anyways.
There was a problem hiding this comment.
Hmmm ya I think we still need to keep an up to date consent list. I'd suggest we handle the caching like we do for conversations in the conversations class https://github.com/xmtp/xmtp-ios/blob/main/Sources/XMTPiOS/Conversations.swift#L57 We could probably open an issue for a follow up if you think that will take too much time. The problem is if that list doesn't reflect correctly calling things like isAllowed which just checks the cached list will return incorrectly. Do they not call any of those methods?
Alternatively in the mean time if they literally just take the list and put it straight into the database we could keep refreshConsentList() as is. That way it's still always resetting the cached list which is what other integrators expect and then add pagination just to the load method and make it public kind of like your first approach. Then they could just call load directly and there would be no expectation of using the convenience methods like isAllowed without first calling refreshConsentList. Does that make sense?
I don't think the caching should take that much time but just giving some options.
alexrisch
force-pushed
the
ar/consent-test
branch
from
6803cc3 to
3b3fb49
Compare
| } | ||
| } | ||
|
|
||
| return consentList | ||
| return Array(entries.values) |
There was a problem hiding this comment.
What does entries.values return if not an array. 🤔
alexrisch
force-pushed
the
ar/consent-test
branch
2 times, most recently
from
fd73890 to
019da92
Compare
Added Caching for Consent List Updated allow/deny calls to only call network once
alexrisch
force-pushed
the
ar/consent-test
branch
from
019da92 to
a1133f0
Compare
Added optional pagination to consent list loading