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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(advanced-logic): use currency manager instance #1268
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I see there are a lot of for cycles when preparing the currencies.
Another way would be to store a static field the CurrencyManager itself, that way you would modify just one file. But this also works.
@@ -2031,10 +2032,19 @@ describe('request-client.js', () => { | |||
}); | |||
|
|||
describe('allows overriding the default currencies', () => { | |||
const currencyManager = new CurrencyManager(CurrencyManager.getDefaultList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test was inaccurate. Previously, the AddressBasedPaymentNetwork
initialize it's own currency manager; hence the request can be created. Suppose that we only have _TEST
currency in the currency manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean? I don't think this was inacurate, why do you need to add ETH?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need ETH to validate the address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah got it. It worked before because the default currency manager was instanciated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. So the instantiated one contains _TEST
currency only, and we have another currency manager that uses default list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strange. why is this empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, not sure 馃し
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's strange that this near-native.ts
appears here in this PR. It lives in the near/
directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we forgot to delete it 馃槵 It's there since a while.
https://github.com/RequestNetwork/requestNetwork/blob/master/packages/advanced-logic/src/extensions/payment-network/near-native.ts
@@ -2031,10 +2032,19 @@ describe('request-client.js', () => { | |||
}); | |||
|
|||
describe('allows overriding the default currencies', () => { | |||
const currencyManager = new CurrencyManager(CurrencyManager.getDefaultList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean? I don't think this was inacurate, why do you need to add ETH?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @KolevDarko , I think it would be easier to save the default instance after the first call to getDefault()
, directly in the CurrencyManager
like so:
export class CurrencyManager {
private static defaultInstance: CurrencyManager;
/**
* Returns a default instance of CurrencyManager based on default lists
*/
static getDefault(): CurrencyManager {
if (this.defaultInstance) return this.defaultInstance;
this.defaultInstance = new CurrencyManager(
CurrencyManager.getDefaultList(),
CurrencyManager.getDefaultLegacyTokens(),
);
return this.defaultInstance;
}
}
This way, we don't have to change any other code. What do you think?
That would work as well @alexandre-abrioux @KolevDarko . However, I still think we have inconsistency issues across payment networks (some are using injected currencies, some are not). And for another reason, to remove dependency with the currency package, as what Benji suggest. |
Ok, that's a good benefit, to decouple the packages. We could do both then, or at least add a comment in the getDefault method that it's heavy |
@KolevDarko @kevindavee I agree lets do both then |
I've open #1269 for another (middle/long term) approach to this issue |
Description of the changes
Context
Initiating a
CurrencyManager
is expensive. I was debugging thecomputeRequestId
function. I'm sharing the logs of my test.Based on my test,
computeRequestId
is executed for 130ms. I'm using a M1 Macbook Pro. External network calls (API/DB calls) only take around 20ms (from verifying signature input until signed). So, we still have 110ms of processing time, and it's all local execution. I suspected two things: for loops or a computing-intensive operation such as signing/hashing/encrypting, etc. But apparently, the process that takes much time isCurrencyManager.getDefault()
. You can see on the logs there are several.getDefault()
calls, and it takes approximately 15ms. A total of 6 calls contribute 90ms from the 110ms local execution 馃ゲChange
Inject instantiated
currencyManager
. I think we should re-use thecurrencyManager
that has been instantiated in theAdvancedLogic
class and pass it down to the payment networks unless there are specific reasons why certain payment networks can't use thecurrencyManager
from theAdvancedLogic
class.