-
Notifications
You must be signed in to change notification settings - Fork 21
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: substrate support to SDK #154
Conversation
examples/substrate-react-example/src/substrate-lib/SubstrateContext.tsx
Outdated
Show resolved
Hide resolved
examples/substrate-react-example/src/substrate-lib/SubstrateContext.tsx
Outdated
Show resolved
Hide resolved
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 took a look particularly at the sdk, went through the tests as well, and skipped the example.
Just left one small comment, there's nothing that really stood out to my eyes, it's pretty clean well done!
return Loader( | ||
"Loading accounts (please review any extension's authorization)" | ||
); | ||
} |
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 code block with error handling could be improved by using constants for the status. It could also be separated into another method for error handling.
What is the possible error state when using "!== READY"? Can we compare the exact values as it gives the feeling of some potential unknown keyring value?
from: string; | ||
to: string; | ||
}): Promise<void> => { | ||
console.log('form data', values); |
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.
Remove console.log
console.log('form data', values); | ||
if (values.amount && values.address) { | ||
setIsLoading(true) | ||
makeDeposit(values.amount, values.address, values.to).finally(() => { |
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.
No error handling
|
||
return ( | ||
<form | ||
action="" |
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.
Empty action could be removed
return ( | ||
<form | ||
action="" | ||
onSubmit={(...args) => void handleSubmit(submit)(...args)} |
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 need a destructuring assignment for the object?
key: account.address, | ||
value: account.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.
use object destructuring
text: (account.meta.name as String).toUpperCase(), | ||
icon: "user", | ||
})); | ||
const initialAddress = |
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.
move to the method getInitialAddress
: ""; | ||
// Set the initial address | ||
useEffect(() => { | ||
!currentAccount && |
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.
refactor to readable expressions, like:
function accountExists() return !currentAccount && initialAddress.length > 0
if(accountExists()) {
setCurrentAccount(keyring?.getPair(initialAddress))
}
) : ( | ||
"No accounts found" | ||
)} |
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.
use the early return of component if an account is not set:
if (!currentAccount) {
return "No accounts found"
}
return (
<div
style={{
display: "flex",
flexDirection: "column",
alignItems: "center",
}}
>
Account data
export default function Metadata(props: any) { | ||
const state = useSubstrateState(); | ||
const api = state?.api; | ||
return api?.rpc && api.rpc.state ? <Main {...props} /> : 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.
if we are relying on the state we can use:
api?.rpc?.state
Or you can use lodash
get(api, 'rpc.state', null)
|
||
export type SubstrateContextType = { | ||
state: StateType; | ||
setCurrentAccount: (acct: unknown) => void; |
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.
should the parameter be account?
|
||
useEffect(() => { | ||
const { apiState, keyringState, api } = state; | ||
if (apiState === "READY" && api && !keyringState && !keyringLoadAll) { |
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.
constants for status like "READY"
useEffect(() => { | ||
const { apiState, keyringState, api } = state; | ||
if (apiState === "READY" && api && !keyringState && !keyringLoadAll) { | ||
console.log("load"); |
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.
remove console.log
); | ||
}; | ||
|
||
const useSubstrate = () => useContext(SubstrateContext); |
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.
make a guard if the context is not used with provider:
function useSubstrate() {
const context = useContext(SubstrateContext)
if (context === undefined) {
throw new Error('useSubstrate must be used within a SubstrateContext')
}
return context
}
systemChain: string; | ||
systemChainType: ChainType; | ||
}> => { | ||
const [systemChain, systemChainType] = await Promise.all([ |
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.
Simplify the code:
const chainPromise = api?.rpc?.system?.chainType ? api.rpc.system.chainType() : Promise.resolve(LiveChainType)
const [systemChain, systemChainType] = await Promise.all([
api.rpc.system.chain()
chainPromise()
])
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.
Approving the PR with the request to add/fix the suggested improvements listed in a new issue:
#181
Description
Related Issue Or Context
Closes: #93
Closes: #92
Closes: #144
Closes: #89
How Has This Been Tested? Testing details.
Types of changes
Checklist: