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: introducing hub, our new wallet management #826

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

yeager-eren
Copy link
Collaborator

@yeager-eren yeager-eren commented Aug 7, 2024

Summary

Adding Hub into our wallets-core.

We've experimented with hub in the original PR and I applied comments as well. In this PR, we will add Hub and its functionality first then in a separate PR we will have an adapter to let us use Hub and legacy system alongside each other.

I tried to have more test coverage and also code documentation in this PR in compare to original PR. I also cleaned up namespace and provider code to have more code quality. One notable change is I renamed and to and_then and also or to or_else to communicate the intention better. Previously, We called and, or, before, after hook actions, but I made into two separate thing, and_then and or_else are (logical) Operators and after and before are hooks.

How did you test this change?

If you need documentation on the concepts, it's available on original PR, other than that I tired to document code using JSDOC as well.

You can review this PR using unit and integration tests that it has, and also I suggest to review in this order:

  1. hub/namespaces
  2. hub/providers
  3. hub/hub.ts
  4. hub/store
  5. /builders
  6. /utils

Blocked by

Related PRs

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • Implemented a user interface (UI) change, referencing our Figma design to ensure pixel-perfect precision.

@yeager-eren yeager-eren marked this pull request as draft August 7, 2024 12:35
@yeager-eren yeager-eren mentioned this pull request Aug 7, 2024
2 tasks
@yeager-eren yeager-eren force-pushed the feat/adding-hub-to-core branch from f2ca87f to 0b5f9be Compare August 8, 2024 08:24
@yeager-eren yeager-eren force-pushed the feat/adding-hub-to-core branch from 0b5f9be to 59f19eb Compare August 15, 2024 15:42
@yeager-eren yeager-eren changed the base branch from next to chore/restructure-shared-and-core-for-hub August 15, 2024 15:42
@yeager-eren yeager-eren force-pushed the feat/adding-hub-to-core branch from 59f19eb to 9edfafe Compare August 16, 2024 11:54
@yeager-eren yeager-eren force-pushed the chore/restructure-shared-and-core-for-hub branch 2 times, most recently from f0b3f07 to 4f06666 Compare August 16, 2024 12:26
@yeager-eren yeager-eren force-pushed the feat/adding-hub-to-core branch from 9edfafe to d3f137c Compare August 16, 2024 12:28
@yeager-eren yeager-eren force-pushed the feat/adding-hub-to-core branch 2 times, most recently from 507d10b to 90e37bc Compare August 17, 2024 07:37
@yeager-eren yeager-eren changed the title WIP! feat: introducing hub, our new wallet management feat: introducing hub, our new wallet management Aug 17, 2024
@yeager-eren yeager-eren marked this pull request as ready for review August 17, 2024 08:05
@yeager-eren yeager-eren force-pushed the feat/adding-hub-to-core branch from 90e37bc to 97222bc Compare August 17, 2024 08:20
Base automatically changed from chore/restructure-shared-and-core-for-hub to next August 19, 2024 06:40
Copy link
Collaborator

@RyukTheCoder RyukTheCoder left a comment

Choose a reason for hiding this comment

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

Great job. I just added some minor comments.

wallets/core/tests/providers.test.ts Outdated Show resolved Hide resolved
wallets/core/tests/providers.test.ts Show resolved Hide resolved
wallets/core/src/namespaces/evm/before.ts Outdated Show resolved Hide resolved
wallets/core/src/namespaces/evm/utils.ts Show resolved Hide resolved
wallets/core/src/hub/namespaces/types.ts Outdated Show resolved Hide resolved
wallets/core/src/hub/namespaces/namespace.ts Show resolved Hide resolved
wallets/core/src/hub/provider/provider.ts Show resolved Hide resolved
wallets/core/src/hub/namespaces/errors.ts Show resolved Hide resolved
wallets/core/src/hub/provider/provider.test.ts Outdated Show resolved Hide resolved
RanGojo

This comment was marked as resolved.

Copy link
Member

@RanGojo RanGojo 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've reviewed EVM and some part of core namespace.

@yeager-eren yeager-eren force-pushed the feat/adding-hub-to-core branch from 1e5a4fd to 7b17ee3 Compare September 18, 2024 10:42
@yeager-eren yeager-eren force-pushed the feat/adding-hub-to-core branch from 7b17ee3 to 5459333 Compare September 18, 2024 10:45
Copy link
Collaborator

@RyukTheCoder RyukTheCoder left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants