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

Joint Privacy: Rework analytics service event management and add signing tracking #3623

Merged
merged 3 commits into from
Sep 7, 2023

Conversation

Shadowfiend
Copy link
Contributor

@Shadowfiend Shadowfiend commented Sep 5, 2023

This PR does two core things:

  • Moves responsibility for more event tracking into the AnalyticsService itself.
  • Adds tracking events for when data is signed and when there is an error signing data or transactions. The data signed and signature are not sent to Posthog, and in the case of errors only a generic set of errors is sent (rather than full error data).

Note that there is still a lot of unnecessary event management happening in Main, which should just be event observations from AnalyticsService. These can be handled at a later time.

Latest build: extension-builds-3623 (as of Wed, 06 Sep 2023 10:24:01 GMT).

@Shadowfiend Shadowfiend requested a review from a team as a code owner September 5, 2023 20:42
AnalyticsService should act as an event sink and manage events
internally to the extent possible. In particular, InternalSignerService
no longer depends on AnalyticsService, but rather vice versa. This sets
the stage for also tracking signing events centrally in
AnalyticsService.
Nothing is recorded about the signature other than the fact that data
was signed. Failures track error reason, which is a very limited set of
possible errors managed by the `SigningService`.

Notably, this commit also moves handling for transaction signatures out
of `Main` and into `AnalyticsService`, which should be loosely coupled
to service events when possible.
Copy link
Contributor

@jagodarybacka jagodarybacka left a comment

Choose a reason for hiding this comment

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

Code looks good, haven't tested if events are actually getting into posthog correctly but let's test it during release testing

🚀

@Shadowfiend Shadowfiend merged commit e19a68a into main Sep 7, 2023
6 checks passed
@Shadowfiend Shadowfiend deleted the joint-privacy branch September 7, 2023 14:14
@Shadowfiend Shadowfiend mentioned this pull request Sep 7, 2023
Shadowfiend added a commit that referenced this pull request Sep 8, 2023
## Highlights
- Fixed an issue where valid private keys would not be accepted. These
always had one or more `0`s at the beginning.
- Fixed an issue where certain types of errors were causing sites not to
load correctly.

## What's Changed
* v0.48.0 by @Shadowfiend in
#3610
* Attempt to import private key to validate it by @jagodarybacka in
#3614
* Group private keys when categorizing by @Shadowfiend in
#3618
* Assert URL of the scan website opened for unverified assets by
@michalinacienciala in #3602
* Handle invalid responses on batch rpc providers by @hyphenized in
#3615
* Joint Privacy: Rework analytics service event management and add
signing tracking by @Shadowfiend in
#3623


**Full Changelog**:
v0.48.0...v0.49.0

Latest build:
[extension-builds-3625](https://github.com/tahowallet/extension/suites/15924368205/artifacts/909765830)
(as of Thu, 07 Sep 2023 20:56:50 GMT).
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.

None yet

2 participants