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

feat: chain agnostic #278

Merged
merged 12 commits into from
Jan 13, 2024
Merged

feat: chain agnostic #278

merged 12 commits into from
Jan 13, 2024

Conversation

chris13524
Copy link
Member

@chris13524 chris13524 commented Jan 11, 2024

Description

  • Implements behavior for treating any account the same as long as it has the same address. This works by using the get_address_lower() function when doing SQL queries.
    • An alternative approach is to migrate the database so that it only stores address and not the full account ID. While this would simplify queries and probably be simpler overall, I decided to keep the current format as this is a risky change overall and the chain info may be needed in the future (for example with smart contract support or namespace identification).
  • For now assumes eip155 account IDs
    • Manually confirmed that all accounts are currently eip155
    • This is enforced going forward with is_eip155_account() in the verify_identity() function
  • Implements migration to delete subscribers with the same address, prior to adding the new unique constraint. This keeps the subscription with the most notifications.
  • Fix security issue where delete subscription requests do not verify that the account matches the looked up subscriber. Added explicit is_same_address() check. This was not a high-risk issue because it required already knowing the symmetric key of the topic, which is private information anyway.
    • Refactor update_subscriber() parameters to take subscriber ID instead of project and account, to simplify, improve performance, and reduce risk of security vulnerabilities (i.e. using the same subscriber that was looked up earlier in the function)
  • Refactor upsert_subscriber() to also return account of the subscriber so that analytics can export a consistent account regardless of which account was used for the request

Resolves #265

How Has This Been Tested?

New automated tests

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@chris13524 chris13524 self-assigned this Jan 11, 2024
@chris13524 chris13524 changed the title feat: cross chain feat: chain agnostic Jan 11, 2024
@chadyj chadyj linked an issue Jan 12, 2024 that may be closed by this pull request
Copy link
Contributor

@geekbrother geekbrother 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 think it's better to migrate the existing database and safer.

Base automatically changed from chore/refactor to main January 12, 2024 13:48
@chris13524 chris13524 marked this pull request as ready for review January 13, 2024 04:10
@chris13524 chris13524 merged commit 2dd50a4 into main Jan 13, 2024
13 checks passed
@chris13524 chris13524 deleted the feat/cross-chain branch January 13, 2024 18:45
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.

Chain agnostic
2 participants