-
Notifications
You must be signed in to change notification settings - Fork 101
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
feat: add static chains config #1103
Conversation
402e4f5
to
b40ea5c
Compare
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.
Nice one! :) I've just left some comments because maybe we won't need this new atom
apps/namadillo/src/registry/index.ts
Outdated
return loadedConfigs[index]; | ||
} | ||
|
||
export function chainConfigByName(name: string): ChainConfig { |
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.
Can we improve name
type to enforce only the available keys?
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 we can define type like:
type ChainName = 'cosmoshub' | 'namada';
At this point I'm not sure though if we will always have every json loaded so it might be undefined anyways.
OR to be more precise at first I thought this type will grow a lot :D but now I'm not sure about this loading thing also.
WDYT?
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 works for now :) let's keep it in mind so we can scale it in a better way
@@ -81,3 +84,7 @@ export const chainParametersAtom = atomWithQuery<ChainParameters>((get) => { | |||
}, | |||
}; | |||
}); | |||
|
|||
export const chainConfigAtom = atomFamily((chainName: string) => |
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.
Do we really need an atom for this?
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.
Nah :)
b40ea5c
to
e06f9f8
Compare
956890c
to
00ae7f7
Compare
00ae7f7
to
16314fe
Compare
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.
Thanks, @mateuszjasiuk! :D LGTM!
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.
LGTM!
This reverts commit dcb53aa.
The most basic stuff.
We can move registry to separate package also