-
Notifications
You must be signed in to change notification settings - Fork 17
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
Multiple RPC endpoint support #2394
Conversation
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
PR deployed in Google Cloud |
PR deployed in Google Cloud |
…r for more reliable rpc connections
(cachedProviders[chainId] = new JsonRpcProvider((evmChains as any)[chainId].urls[0], chainId)) | ||
) | ||
async function findHealthyProvider(chainId: number, urls: string[]): Promise<JsonRpcProvider> { | ||
for (const url of urls) { |
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.
Would use a Promise.any here
if (!cachedProviders.has(chainId)) { | ||
findHealthyProvider(chainId, urls).catch(console.error) | ||
} | ||
return cachedProviders.get(chainId) || new JsonRpcProvider(urls[0], chainId) |
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.
with findHealthyProvider
being async, this may return a new JsonRpcProvider
multiple times and not garantee that it's healthy
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've changed the implementation to now use ethers getDefaultProvider()
its, much cleaner and doesn't require the manual work from before
const calls: EvmMulticallCall[] = [] | ||
const toTokenBalance = (val: BigNumber) => new TokenBalance(val.toString(), 18) | ||
const toCurrencyBalance = (val: BigNumber) => new CurrencyBalance(val.toString(), 18) | ||
const toTokenBalance = (val: BigInt) => new TokenBalance(val.toString(), 18) |
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.
BigInt as a type has to be lowercase everywhere
centrifuge-js/src/CentrifugeBase.ts
Outdated
} | ||
|
||
private async findHealthyWs(): Promise<string | null> { | ||
for (const url of this.rpcEndpoints) { |
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.
would use Promise.any here
centrifuge-js/src/CentrifugeBase.ts
Outdated
clearTimeout(timer) | ||
ws.close() | ||
console.log(`Connection to ${url} failed`) | ||
resolve(false) |
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.
if using Promise.any above, you can reject here instead
centrifuge-js/src/CentrifugeBase.ts
Outdated
@@ -295,19 +296,69 @@ type Events = ISubmittableResult['events'] | |||
|
|||
const txCompletedEvents: Record<string, Subject<Events>> = {} | |||
const blockEvents: Record<string, Observable<Events>> = {} | |||
let parachainUrlCache: string | null = null |
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.
this won't work if there's different instances of the Centrifuge class. It's why the above caches are keyed by parachain url. I think it would be better to keep this as a property on the class. It would need to be copied over when the instance gets cloned though, like in Centrifuge->connect()
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 agreed initially that the variable should be stored as a class property, but I noticed in doing so that the url is actually not cached at all and each time the instance is recreated the class property loses it's value. Like this we can store the value across instances which IMO is what we want for this feature instead of constantly opening a ws to check it's health and closing it again. Like this the cached url will only be reset when the page reloads and we can limit the number of ws opening requests to max as many urls as we have
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.
what about making this a Set
reachableParachainUrls
? Then when getting the cached url, it can check which of the config urls is in the set. Then it still works if there's multiple instances of Centrifuge with different configs
centrifuge-js/src/CentrifugeBase.ts
Outdated
@@ -459,7 +510,13 @@ export class CentrifugeBase { | |||
}), | |||
tap((result) => { |
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.
The tap
callback can be an async function and then this.getTxCompletedEvents()
can simply be awaited
centrifuge-js/src/CentrifugeBase.ts
Outdated
from(this.getTxCompletedEvents()) | ||
.pipe(take(1)) | ||
.subscribe((subject) => { | ||
subject.next(result.events) | ||
}) |
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.
from(this.getTxCompletedEvents()) | |
.pipe(take(1)) | |
.subscribe((subject) => { | |
subject.next(result.events) | |
}) | |
(await this.getTxCompletedEvents()).next(result.events) |
@@ -373,7 +381,7 @@ export function getLiquidityPoolsModule(inst: Centrifuge) { | |||
...(currencies.flatMap((currency) => ({ | |||
target: poolManager, | |||
call: ['function assetToId(address) view returns (uint128)', currency.address], | |||
returns: [[`currencyNeedsAdding[${currency.address}]`, (id: BigNumber) => id.isZero()]], | |||
returns: [[`currencyNeedsAdding[${currency.address}]`, (id: BigInt) => id === BigInt(0)]], |
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.
returns: [[`currencyNeedsAdding[${currency.address}]`, (id: BigInt) => id === BigInt(0)]], | |
returns: [[`currencyNeedsAdding[${currency.address}]`, (id: bigint) => id === 0n]], |
centrifuge-js/src/modules/tinlake.ts
Outdated
@@ -94,6 +93,7 @@ export function getTinlakeModule(inst: Centrifuge) { | |||
) { | |||
const [tranche] = args | |||
return pending( | |||
// @ts-ignore |
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.
what are all these errors saying?? 😅😅😅
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.
The types just weren't inferred properly by the connected contract. When using the the ethers Contract
directly the methods on the contract are not type checked, but when using a connected contract the are type check even though the method types seem to be non-inferable from the ABI.
centrifuge-js/src/CentrifugeBase.ts
Outdated
if (parachainUrlCache.values().next().value) { | ||
return parachainUrlCache.values().next().value |
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.
if (parachainUrlCache.values().next().value) { | |
return parachainUrlCache.values().next().value | |
const match = this.rpcEndpoints.find(url => parachainUrlCache.has(url)) | |
if (match) { | |
return match |
@sophialittlejohn Looking good! One last comment. |
Description
This pull request...
getDefaultNetwork
for centrifuge-react connectors2164
Approvals
Screenshots
Impact