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.

Sign up for GitHub

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

Jump to bottom

feat: add PreEventCallback #144

Merged
merged 16 commits into from
Dec 16, 2023
Merged

feat: add PreEventCallback #144

merged 16 commits into from
Dec 16, 2023

Conversation

kele-leanes
Copy link
Contributor

@kele-leanes kele-leanes commented Dec 5, 2023 •
edited by cameronvoell
Loading

Description:
This PR introduces the preEnableIdentityCallback and preCreateIdentityCallback to the ClientOptions object to be called before enabling and creating an identity.
This feature is the starting point of this issue

@kele-leanes kele-leanes self-assigned this Dec 5, 2023
giovasdistillery
giovasdistillery approved these changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

@giovasdistillery giovasdistillery left a comment

Choose a reason for hiding this comment

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

LGTM, I just want to know why the use of @Ignore("CI issues")

library/src/androidTest/java/org/xmtp/android/library/ClientTest.kt Outdated
@@ -85,6 +89,7 @@ class ClientTest {
}

@Test
@Ignore("CI Issues")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why is using this @Ignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as you can see here we can not run a local environment on CI. is the reason why most of the test that requires create a Client are ignored

Copy link
Contributor

Choose a reason for hiding this comment

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

You actually can run in the local environment in CI. IOS is the only repo who has that issue. You can see more of how this is setup here https://github.com/xmtp/xmtp-android/blob/main/.github/workflows/test.yml#L51

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so why was the test pass in my local and it didn’t in CI? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

@kele-leanes can you link me to the CI that failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/xmtp/xmtp-android/actions/runs/7106161017/job/19344969828 It seems there is an error creating the client

@kele-leanes kele-leanes marked this pull request as ready for review December 5, 2023 19:15
@kele-leanes kele-leanes requested a review from a team as a code owner December 5, 2023 19:15
@nplasterer
Copy link
Contributor

Do you mind writing up a detailed issue body so future developers can understand the changes made here?

nplasterer
nplasterer reviewed Dec 5, 2023
View reviewed changes
Copy link
Contributor

@nplasterer nplasterer left a comment

Choose a reason for hiding this comment

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

@kele-leanes Do you mind writing up how you see this working in React Native. It's just hard to see the whole picture here and would like more information how you envision this threading together. 🙏

library/src/androidTest/java/org/xmtp/android/library/ClientTest.kt Outdated Show resolved Hide resolved
library/src/androidTest/java/org/xmtp/android/library/ClientTest.kt Show resolved Hide resolved
library/src/androidTest/java/org/xmtp/android/library/ClientTest.kt Outdated Show resolved Hide resolved
@kele-leanes kele-leanes mentioned this pull request Dec 13, 2023
Merged
@kele-leanes
Copy link
Contributor Author

@nplasterer
The purpose of this pull request is to add the optional methods preCreateIdentityCallback and preEnableIdentityCallback, which are received through the ClientOptions and are invoked in the create method. These methods are executed just before creating an identity in the case of preCreateIdentityCallback when calling wallet.createIdentity(), and in the case of preEnableIdentityCallback, they are called both when encrypting or decrypting a keyBundle.
This is necessary for integration with React Native, as on the JavaScript side, we may be listening to events (exposed here) that are triggered once these callbacks are invoked.

@nplasterer
Copy link
Contributor

Thanks @kele-leanes for the explanation and working code in RN. I'll give this another pass later today on the tests and get it merged for you.

nplasterer
nplasterer approved these changes Dec 16, 2023
View reviewed changes
Copy link
Contributor

@nplasterer nplasterer left a comment

Choose a reason for hiding this comment

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

Just did a small clean up of some of the methods 👍

@nplasterer nplasterer merged commit e02a8c6 into main Dec 16, 2023
6 checks passed
@nplasterer nplasterer deleted the kele/pre-identity-callback branch December 16, 2023 05:34
@nplasterer nplasterer mentioned this pull request Dec 16, 2023
Merged
@cameronvoell cameronvoell mentioned this pull request Dec 22, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giovasdistillery giovasdistillery giovasdistillery approved these changes

@nplasterer nplasterer nplasterer approved these changes

Assignees

@kele-leanes kele-leanes

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kele-leanes @nplasterer @giovasdistillery