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

Consent V3 #297

Merged
merged 11 commits into from
Sep 16, 2024
Merged

Consent V3 #297

merged 11 commits into from
Sep 16, 2024

Conversation

nplasterer
Copy link
Contributor

@nplasterer nplasterer commented Sep 13, 2024

Part of xmtp/libxmtp#801

This adds the ability to get and set consent records in V3

Dual sends so we are always setting both V2 and V3 consent. And adds a check to see if a V2Client is present to support pure V3 clients.

@nplasterer nplasterer self-assigned this Sep 13, 2024
if (client.v3Client != null) {
setV3ConsentState(entries)
}
if (client.hasV2Client) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in prep for when we have SCW. Only publish if we have a V2 client.

Comment on lines 250 to 256
entries.forEach {
client.v3Client?.setConsentState(
ConsentState.toFfiConsentState(it.consentType),
EntryType.toFfiConsentEntityType(it.entryType),
it.value
)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For performance reasons in this interim period of V2 and V3 we might want to add a FFibinding for setting a list so we dont have to iterate.

@@ -270,7 +346,8 @@ data class Contacts(
) {

suspend fun refreshConsentList(): ConsentList {
consentList.load()
val entries = consentList.load()
consentList.setV3ConsentState(entries)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After loading the V2 consent add them all to the V3 database.

Comment on lines +301 to 311
suspend fun state(address: String): ConsentState {
client.v3Client?.let {
return ConsentState.fromFfiConsentState(
it.getConsentState(
FfiConsentEntityType.ADDRESS,
address
)
)
}
val entry = entries[ConsentListEntry.address(address).key]

return entry?.consentType ?: ConsentState.UNKNOWN
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously it was checking this local entries list which we could still just do since we write to both. But I like move towards checking the V3 state only since that should be the ultimate source of truth if you have V3.

@nplasterer nplasterer marked this pull request as ready for review September 13, 2024 23:58
@nplasterer nplasterer requested a review from a team as a code owner September 13, 2024 23:58
var dbPath: String = ""
lateinit var inboxId: String
var hasV2Client: Boolean = false
Copy link
Contributor

@cameronvoell cameronvoell Sep 14, 2024

Choose a reason for hiding this comment

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

Just a nit - looks like it is currently impossible to create a Client without hasV2Client being true, but it is pretty hard to tell that from this code. (you need to check that all functions that return Client either call buildFromV1Bundle or loadOrCreateKeys)

Since currently all code paths set this to true, I wonder if we should just set it to true by default, instead of false, and add the hasV2Client = false later on when there is a path that actually makes this possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm you know this is a good point. I actually think it's going to be really hard to validate the correctness of all our work without having the ability to make a pure v3 client. I'm going to open a separate PR today that so that we can make certain this stuff is working like expected in V3 and not just a side effect of the dual sending.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From creating this PR I align with you it should be defaulted to true https://github.com/xmtp/xmtp-android/pull/298/files

@cameronvoell cameronvoell self-requested a review September 14, 2024 01:18
Copy link
Contributor

@cameronvoell cameronvoell left a comment

Choose a reason for hiding this comment

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

🚀

@nplasterer nplasterer merged commit 12a55d8 into main Sep 16, 2024
5 of 6 checks passed
@nplasterer nplasterer deleted the np/dual-send-consent-v3 branch September 16, 2024 23:26
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.

2 participants