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

Update Communicator to include app metadata #1385

Merged
merged 1 commit into from
Sep 5, 2024
Merged

Conversation

cb-jake
Copy link
Contributor

@cb-jake cb-jake commented Sep 5, 2024

Summary

This change initializes the communicator with the app metadata. This provides app metadata to the onboarding screen on keys.coinbase.com

How did you test your changes?

Testing these changes requires running a few apps:

  • Playground
  • SCW.
  1. build a version of the sdk and install it into the playground. Make sure the playground is using the newly built version
  2. run the playground and scw locally (place a log where the communicator is initialized)
  3. Check the message event from the sdks communicator and validate appName, and appLogoUrl are available inside of scw.

setting-metadata-scw

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Sep 5, 2024

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@cb-jake cb-jake requested a review from fan-zhang-sv September 5, 2024 20:29
@cb-jake cb-jake force-pushed the communicator-metadata branch 3 times, most recently from c079821 to 3b7737b Compare September 5, 2024 20:34
@cb-jake cb-jake force-pushed the communicator-metadata branch from 3b7737b to c6e11a7 Compare September 5, 2024 20:35
@cb-jake cb-jake requested a review from bangtoven September 5, 2024 20:53
this.url = new URL(url);
this.metadata = metadata;
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder whether this needs to be a param of waitForPopupLoaded method, not in constructor level.
(btw, we might rename the method to be, initialize or connect maybe??)
@cb-jake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bangtoven If we made it a param of waitForPopupLoaded we would probably need to make it a param postMessage as well which would require passing it on calls to postMessage. Currently we call waitForPopupLoaded in 2 places, though one of those places is inside the communicators postmessage method which is called in 4 spots. I'm not positive we would have access to the metadata in all spots we call these methods

Copy link
Contributor

@bangtoven bangtoven left a comment

Choose a reason for hiding this comment

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

we should revisit Communicator's implementation so that we can understand when it should pass metadata to popup. probably only for onboarding or handshake, not all messages.

@cb-jake cb-jake merged commit e355b37 into master Sep 5, 2024
6 checks passed
@cb-jake cb-jake deleted the communicator-metadata branch September 5, 2024 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants