Skip to content

refactor: currency types - #1238

Merged
benjlevesque merged 4 commits into
masterfrom
refactor/currency-types
May 6, 2024
Merged

refactor: currency types#1238
benjlevesque merged 4 commits into
masterfrom
refactor/currency-types

Conversation

@benjlevesque

@benjlevesquebenjlevesque commented Nov 14, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Move all types to the types package for better consistency with other types and as a first step to reduce unnecessary dependencies to the currency package.

BREAKING: types must be updated with CurrencyTypes prefix, imported from @requestnetwork/types package.
BREAKING: the currencies field has been removed from the RequestNetwork client parameters. Use currencyManager: new CurrencyManager(CURRENCIES) instead.

@benjlevesquebenjlevesque changed the title refactor/currency typesrefactor: currency typesNov 14, 2023
@benjlevesque
benjlevesque changed the base branch from master to refactor/remove-axiosNovember 14, 2023 10:59
benjlevesque
benjlevesque commented Nov 14, 2023
View reviewed changes
Comment threadpackages/advanced-logic/src/extensions/payment-network/near/any-to-near-testnet.ts Outdated
benjlevesque
benjlevesque commented Nov 14, 2023
View reviewed changes
Comment threadpackages/currency/src/chains/evm/data/bsc.ts Outdated
benjlevesque
benjlevesque commented Nov 14, 2023
View reviewed changes
Comment threadpackages/currency/src/chains/evm/data/avalanche.ts Outdated
benjlevesque
benjlevesque commented Nov 14, 2023
View reviewed changes
Comment threadpackages/request-client.js/src/http-request-network.ts
nodeConnectionConfig?: Partial<NodeConnectionConfig>;
signatureProvider?: SignatureProviderTypes.ISignatureProvider;
useMockStorage?: boolean;
currencies?: CurrencyInput[];

@benjlevesquebenjlevesqueNov 14, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Breaking: I suggest to remove this parameter as it's redundant with currencyManager

@MantisCloneMantisCloneNov 14, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agreed it's redundant, but is the currencyManager the better choice? I'm not super familiar.

@benjlevesquebenjlevesqueNov 14, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

between the two? I would say yes: it enables injecting your own implementation, and the CurrencyManager contains more than just a list of currencies. For instance, it contains the list of Aggregators for conversion

@MantisCloneMantisCloneNov 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay. 👍 Thanks for explaining

@benjlevesquebenjlevesque mentioned this pull request Nov 28, 2023
Merged
@benjlevesque
benjlevesqueforce-pushed the refactor/remove-axios branch 2 times, most recently from d68636a to 8e03773CompareJanuary 18, 2024 10:12
Base automatically changed from refactor/remove-axios to masterJanuary 18, 2024 10:18
@benjlevesque
refactor: currency types
c165aba
Apply suggestions from code review
@benjlevesque
benjlevesqueforce-pushed the refactor/currency-types branch from 060bdc0 to c165abaCompareApril 23, 2024 08:58
@benjlevesque
benjlevesque marked this pull request as ready for review April 23, 2024 09:54
@benjlevesque
benjlevesque merged commit ad9af1b into masterMay 6, 2024
@benjlevesque
benjlevesque deleted the refactor/currency-types branch May 6, 2024 08:05
@MantisCloneMantisClone mentioned this pull request Jul 22, 2024
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MantisCloneMantisCloneAwaiting requested review from MantisClone
@yomarionyomarionAwaiting requested review from yomarion
2 more reviewers
@alexandre-abriouxalexandre-abriouxalexandre-abrioux approved these changes
@leoslrleoslrleoslr approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@benjlevesque@MantisClone@alexandre-abrioux@leoslr