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

Add provider strategy #46

Merged
merged 23 commits into from
Dec 16, 2024
Merged

Add provider strategy #46

merged 23 commits into from
Dec 16, 2024

Conversation

mgavrila
Copy link

Proposal

  • Change function based provider creation into classes.
  • Provider creation logic is contained inside the class.

Usage:
const providerInstance = new WalletConnectProviderStrategy( walletConnect );
const provider = await providerInstance.createProvider();

src/core/providers-strategy/IFrameProviderStrategy.ts Outdated Show resolved Hide resolved
src/core/providers-strategy/IFrameProviderStrategy.ts Outdated Show resolved Hide resolved
src/core/providers/ProviderFactory.ts Outdated Show resolved Hide resolved
src/core/providers/ProviderFactory.ts Outdated Show resolved Hide resolved
src/core/providers-strategy/CustomProviderStrategy.ts Outdated Show resolved Hide resolved
src/core/providers-strategy/CrossWindowProviderStrategy.ts Outdated Show resolved Hide resolved
src/core/providers-strategy/CrossWindowProviderStrategy.ts Outdated Show resolved Hide resolved
src/core/providers/ProviderFactory.ts Outdated Show resolved Hide resolved
src/core/providers-strategy/CustomProviderStrategy.ts Outdated Show resolved Hide resolved
src/core/providers-strategy/CustomProviderStrategy.ts Outdated Show resolved Hide resolved
src/core/providers-strategy/CustomProviderStrategy.ts Outdated Show resolved Hide resolved
src/core/providers-strategy/CustomProviderStrategy.ts Outdated Show resolved Hide resolved
src/core/providers-strategy/CustomProviderStrategy.ts Outdated Show resolved Hide resolved
src/core/providers/ProviderFactory.ts Outdated Show resolved Hide resolved
src/types/provider.types.ts Outdated Show resolved Hide resolved
throw new Error(ProviderErrorsEnum.notInitialized);
}

if (!isBrowserWithPopupConfirmation) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did this should be configurable since we don't know all the browsers and maybe the user is using a different browser and needs the pop-up confirmation for it

) => Promise<Transaction[]>)
| null = null;

constructor(address: string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you actually need addressIndex from the store

throw err;
}
if (eventBus) {
eventBus.unsubscribe(
Copy link
Collaborator

Choose a reason for hiding this comment

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

eventbus?.unsubscribe ?


const hwProviderLogin = provider.login;
const provider = this.provider as unknown as IProvider;
provider.setAccount({ address: this.address });
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should also set addressIndex in HWProvider

document.body.appendChild(signModalElement);
await customElements.whenDefined('sign-transactions-modal');
if (shouldInitiateLogin) {
eventBus = await getEventBus('ledger-connect-modal');
Copy link
Collaborator

Choose a reason for hiding this comment

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

there should always be a one-way flow between opening the modal and getting the event bus. I can't see the need of a split

+ there is a different eventBus for signing transactions

createdProvider.getType = () => ProviderTypeEnum.ledger;

await createdProvider.init?.();
const providerInstance = new LedgerProviderStrategy(address);
Copy link
Collaborator

Choose a reason for hiding this comment

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

same question for all


createdProvider = provider as unknown as IProvider;
const providerInstance = new WalletConnectProviderStrategy(
walletConnectConfig
Copy link
Collaborator

Choose a reason for hiding this comment

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

can this be fetched as a default from store inside the strategy?

@@ -42,13 +41,6 @@ export interface IProviderConfig {
address: string;
};
UI?: IProviderConfigUI;
Copy link
Collaborator

Choose a reason for hiding this comment

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

UI should be removed

src/utils/modalEvent.ts Outdated Show resolved Hide resolved
import { safeWindow } from 'constants/index';
import { defineCustomElements } from 'lib/sdkDappCoreUi';

export const getModalElement = async <
Copy link
Collaborator

Choose a reason for hiding this comment

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

createModalElement?

@@ -16,6 +18,8 @@ type BaseDappConfigType = {
* default: `true`
*/
enableTansactionTracker?: boolean;
crossWindow?: CrossWindowConfig;
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we make like providers: {walletconnect, crossWindow } ?

private _signMessage: ((messageToSign: Message) => Promise<Message>) | null =
null;

constructor(address?: string, walletAddress?: string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

initialize with props? : {address?:string, ...

withEventBus?: boolean;
};

export const createModalElement = async <
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename file to createModalElement

private _signMessage: ((messageToSign: Message) => Promise<Message>) | null =
null;

constructor({ address = '', walletAddress }: CrossWindowProviderProps) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

props: CrossWindowProviderProps = ...
otherwise you have to do constructor({}}

@mgavrila mgavrila merged commit 8a312d4 into development Dec 16, 2024
1 check failed
@mgavrila mgavrila deleted the amg_provider_strategy branch December 16, 2024 11:59
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.

3 participants