DeXter-on-Radix / website

Community built order book dex on Radix.
https://dexteronradix.com/
Other
13 stars 21 forks source link

Resolve multiple connections to AlphaDEX SDK #41

Closed fliebenberg closed 1 year ago

fliebenberg commented 1 year ago

It seems the app is connecting multiple times to the AlphaDEX SDK. We need to look at the reason and attempt to solve it.

fliebenberg commented 1 year ago

Not sure if anything changed or if it was just something that happenend during development, but it seems the issue has been resolved. Before closing the issue, @SmashingBumpkin I just want to check, why did you need to move the adex.init() out of a useEffect? I am still worried that it might lead to multiple connections to the SDK once we start having multiple pages in the app. There might be a different way of accomplishing what you wanted to do while still keeping the adex.init in a useEffect.

EngineerCharlie commented 1 year ago

So, yes I think there is a problem with moving adex.init() in useEffect.

adex.init(); //Connect to alphadex websocket

let initialStaticState = new adex.StaticState(adex.clientState.internalState);

export const AdexStateContext = createContext(initialStaticState);

AdexStateContext needs to be shared with lower level components, and for that I think it needs to be exported. To export it it can't be in useEffect. To create it as a constant it needs initialStaticState, and to create that the adex connection needs to be initialized.

Of course, there could be another way to do all this! But, it's not as trivial as just moving adex.init() into useEffect.

fliebenberg commented 1 year ago

adex.init() only handles the connection with the websocket. The adex.clientSTate is created up-front in the SDK with static data, so you can do the initialStaticSTate and AdexSTateContext assignments without running adex.init(). If you remove adex.init() from its current position and put it at the top of the useEffect it should still work. You can even test leaving out adex.init() completely and everything should work. You will just not see any real data from AlphaDEX as it will only be displaying the initial static values that is populated as placeholders until real data can be loaded.

fliebenberg commented 1 year ago

Also, you can replace the adex.clientState.status in the getAdexConnectionStatus function with adexState.status. We should avoid using adex.clientState anywhere in the app as it will not get updated on screen as expected. The only places we need to reference adex directly is when calling functions.

EvgeniiaVak commented 1 year ago

"We should avoid using adex.clientState anywhere in the app as it will not get updated on screen as expected." Ok, this is good to know, maybe we even need a comment in the code about it somewhere.

EvgeniiaVak commented 1 year ago

@SmashingBumpkin

export const AdexStateContext = createContext(initialStaticState);

Does this work ok for you if you build? (npm run build) next.js complains about wrong exports a lot.

EvgeniiaVak commented 1 year ago

@fliebenberg might it have been something that I've done in PriceChart and then fixed in 2a750a150228a1ada3b408a174096f3ae788f40a ? I don't call init there but I do import { AdexStateContext } from "./page";

EngineerCharlie commented 1 year ago

@SmashingBumpkin

export const AdexStateContext = createContext(initialStaticState);

Does this work ok for you if you build? (npm run build) next.js complains about wrong exports a lot.

The build fails like so:

- info Linting and checking validity of types .Failed to compile.

.next/types/app/page.ts:8:13
Type error: Type 'OmitWithTag<typeof import("C:/Users/SB/Programming/website/src/app/page"), "metadata" | "default" | "config" | "generateStaticParams" | "revalidate" | "dynamic" | "dynamicParams" | "fetchCache" | "preferredRegion" | "runtime" | "generateMetadata", "">' does not satisfy the constraint '{ [x: string]: never; }'.
  Property 'AdexStateContext' is incompatible with index signature.
    Type 'Context<StaticState>' is not assignable to type 'never'.

   6 |
   7 | // Check that the entry is a valid entry
>  8 | checkFields<Diff<{
     |             ^
   9 |   default: Function
  10 |   config?: {}
  11 |   generateStaticParams?: Function
fliebenberg commented 1 year ago

I am also getting an error when I run npm run build on the "generate-orders" branch. Similar error, but mine says it relates to rdt on the layout page. next/types/app/layout.ts:8:13 Type error: Type 'OmitWithTag<typeof import("C:/Fred/Coding/dexter/website/src/app/layout"), "metadata" | "default" | "config" | "generateStaticParams" | "revalidate" | "dynamic" | "dynamicParams" | "fetchCache" | "preferredRegion" | "runtime" | "generateMetadata", "">' does not satisfy the constraint '{ [x: string]: never; }'. Property 'rdt' is incompatible with index signature. Type '{ requestData: (value: { accounts?: ({ quantifier: "exactly" | "atLeast"; quantity: number; } & { oneTime?: boolean | undefined; reset?: boolean | undefined; }) | undefined; personaData?: ({ ...; } & { ...; }) | undefined; }) => ResultAsync<...>; ... 4 more ...; destroy: () => void; }' is not assignable to type 'never'.

fliebenberg commented 1 year ago

I am not too familiar with react and context, but I see there are quite a few different hooks and something called rdtContext that you created @SmashingBumpkin . When I look further into the error message it looks like some of the fields mentioned relate to RDT. Might it be that something goes wrong in all the importing and exporting of the different contexts/hooks? Ill continue to dig around a bit. It looks like there might be a problem with how we import/export states between components.

EngineerCharlie commented 1 year ago

I think you're right, specifically it seems to be related to anything exported (which is why it picks up AdexStateContext as well).

At first I thought it was type related, but assigning types doesn't resolve it, even assigning the type as "any" doesn't really help.

p.s. This has gone quite off-topic. @fliebenberg just to confirm with adex.init() in useEffect you aren't getting flooded with connections?

fliebenberg commented 1 year ago

At the moment there are not a large number of clients connected, but to be honest that issue seemed to be fixed even before we moved the adex.init() into useEffect. But I still think it is better to have it in the useEffect if it does not cause any other issues.

fliebenberg commented 1 year ago

I think you're right, specifically it seems to be related to anything exported (which is why it picks up AdexStateContext as well).

At first I thought it was type related, but assigning types doesn't resolve it, even assigning the type as "any" doesn't really help.

p.s. This has gone quite off-topic. @fliebenberg just to confirm with adex.init() in useEffect you aren't getting flooded with connections?

I am working on a solution for this. It is indeed related to the exporting of the useEffect const. The solution is to move the useEffect into its own file that you can then import, but then you have to link it up with the useState variables for adexState and some of the rdt fields like accounts and persona. But I have a working solution and am working on a PR for the main branch to do this. Will hopefully post tonight still.

fliebenberg commented 1 year ago

I just cant get a clean build. I am pretty sure I have sorted out the structure of the state variable that can be passed through useContext, but I am still not getting a clean build. It is still giving the old error of not able to find "document" which is what happen when it tries to render the pages on the server.

EvgeniiaVak commented 1 year ago

@fliebenberg could you please share the version you are trying to build (maybe in another branch)? I would like to have a look and fix this error, and I cannot reproduce it in main.

fliebenberg commented 1 year ago

Not sure if I mentioned it, but I pushed my branch called : fix-build-error

EvgeniiaVak commented 1 year ago

I guess this one is also resolved by #46