-
Notifications
You must be signed in to change notification settings - Fork 28
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(types): add custom types for authorities, plugins and asset #9
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.
This looks great! I'll leave it to @nhanphan to approve since he works more with the devex side of things.
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.
Overall Asset structure looks good, only had minor questions/comments
clients/js/src/hooked/types.ts
Outdated
export type UpdateDelegatePlugin = BasePlugin & UpdateDelegate; | ||
|
||
export type PluginsList = { | ||
reserved?: ReservedPlugin; |
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 should hide this (i.e. skip it, it should not appear on the asset)
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.
Done. 3e741c6
Though, I'm not sure what a more convenient way to handle this with mapping functions would be. I have 2 options:
- For now, I restrict that on the type level. It means that when using map helpers and working with Kinobi-generated
Plugin
type we will have to check if a plugin is not of aReserved
kind. (e.g.registryRecordsToPluginsList()
) - We might allow accept the
Plugin
type as is, but then we will have to returnnull
/undefined
, when mapping a plugin. And if it's used in loops, we'll have to filter thosenull
/undefined
.
Do you have any thoughts on that?
permanent?: Array<PublicKey>; | ||
}; | ||
|
||
export type BasePlugin = { |
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 should add offset
as well. IIRC it's in the registry per plugin just in case anyone wants to do binary stuff with the underlying uint8 array
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.
Done. 3e741c6
}; | ||
} | ||
|
||
export function formPluginRegistry({ |
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.
when would you imagine we use formPluginRegistry
and formPluginWithAuthorities
since I think the original thought was to use them for the t.like(asset,...
things. Were you thinking that we'd also use them to submit instructions via say create
or something?
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 written the explanations on this topic in Slack. Let me copy it here:
So, if we go with custom types we can probably drop a couple of plugin helpers, namely
formPluginRegistry()
andformPluginWithAuthorities()
since we won't have the registry field and theAssetWithPlugins
type will have a different structure.formPluginHeader()
might still be useful, because we decided to leave thepluginHeader
field.
I don't see use cases for that couple of helpers yet. Removed those for now. 3e741c6
import { AssetWithPluginsTest, PluginsList } from './types'; | ||
import { registryRecordsToPluginsList } from './pluginHelpers'; | ||
|
||
export async function fetchAssetWithPluginsTest( |
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.
@blockiosaurus thoughts on just straight overriding fetchAsset with this? i.e. the client lib exposes this as fetchAsset and don't expose the generated fetchAsset?
is there any reason not to deserialize the whole thing always since it's in a single account?
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'm conflicted. I do like having an explicit fetch with plugins but maybe that'll just be confusing.
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.
Is it possible, that someone would like to fetch an asset without plugins?
d3dc376
to
3e741c6
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.
besides feedback from @blockiosaurus on that one comment, this looks good to me. we should replicate the same thing (in another PR/change) to collection as well.
3e741c6
to
143e99b
Compare
The old P.S. Couldn't use |
Description
fetchAsssetWithPlugins()
is added