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: cp-8359 chainagnostic provider #6

Merged
merged 14 commits into from
Aug 22, 2024
Merged

feat: cp-8359 chainagnostic provider #6

merged 14 commits into from
Aug 22, 2024

Conversation

vvava
Copy link
Contributor

@vvava vvava commented Jul 29, 2024

Description

We want to create a Chain Agnostic Provider to unify the different providers.

Changes

Implement the provider and use it in CoreProvider

FYI: There are several test cases which are failing in CoreProvider I'm skipping at the moment. Later I will revisit and fix them all.

Testing

Try to use a dApp, everything should work as before.

Screenshots:

Screen.Recording.2024-07-22.at.10.04.05.mov

Checklist for the author

Tick each of them when done or if not applicable.

  • I've covered new/modified business logic with Jest test cases.
  • I've tested the changes myself before sending it to code review and QA.

ryanml
ryanml previously approved these changes Jul 29, 2024
View reviewed changes
Copy link
Contributor

@ryanml ryanml left a comment •
edited
Loading

Choose a reason for hiding this comment

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

code lgtm, will follow up additionally and test

@meeh0w
Copy link
Member

meeh0w commented Jul 31, 2024 •
edited
Loading

We just gotta fix the failing tests and I'll hit approve 👍

@vvava vvava force-pushed the feat/CP-8359_create_chaingagnostiv branch from 94c1cb5 to 80a305b Compare August 1, 2024 14:55
meeh0w
meeh0w previously approved these changes Aug 2, 2024
View reviewed changes
Copy link
Member

@meeh0w meeh0w left a comment

Choose a reason for hiding this comment

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

Left two nitpicks, looks good otherwise 👍

src/background/providers/ChainAgnosticProvider.ts Outdated Show resolved Hide resolved
src/background/providers/ChainAgnosticProvider.ts Outdated Show resolved Hide resolved
@vvava vvava requested a review from meeh0w August 21, 2024 15:57
@vvava vvava requested a review from gergelylovas August 22, 2024 12:58
@vvava vvava merged commit 38c2ef1 into main Aug 22, 2024
5 checks passed
@vvava vvava deleted the feat/CP-8359_create_chaingagnostiv branch August 22, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bferenc bferenc bferenc approved these changes

@gergelylovas gergelylovas gergelylovas approved these changes

@ryanml ryanml ryanml left review comments

@meeh0w meeh0w Awaiting requested review from meeh0w

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vvava @meeh0w @ryanml @gergelylovas @bferenc