Skip to content

Conversation

@lorisleiva
Copy link
Member

This PR uses the Kinobi IDL standard to describe the Config program.

Notice it uses features such as shortU16 numbers, "either" signers and remaining account nodes which are not supported by the Anchor IDL standard.

@lorisleiva lorisleiva self-assigned this Apr 17, 2024
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great overall! Just a few comments, which I hope that @buffalojoec can keep me honest on

@joncinque joncinque requested a review from buffalojoec April 17, 2024 12:41
@lorisleiva lorisleiva requested a review from joncinque April 17, 2024 13:40
Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks great! However, I have some thoughts about using this as the official program specification.

What's Great

A program's specification, to be referenced in protocol SIMDs and elsewhere, should be completely agnostic to any implementation. It should be verbose and descriptive, but also somewhat digestible and trivial to implement in any programming language (particularly JavaScript, Rust or C).

I have a really hard time identifying anything in this IDL that's not relevant to the program's specification. It describes how to encode data (instructions, accounts), instruction keys and their roles, types, and it even goes so far as to describe other programs this program may depend on.

The documentation bits are superb, and I especially like the overall verbosity of everything.

The best part about this is that we can obviously generate Kinobi-based clients, but we can also do even more. We could use this to generate test fixtures for frameworks like Solana Program Test, Boomerang, and SolFuzz.

What's Missing

I originally had qualms about the fact that this IDL is a literal Abstract Syntax Tree, rather than an interface (even though a valid argument can be made to say "what's the difference?"). However, the more I think about it, the more I think this is a good thing.

If we're going with the Kinobi standard for specifying on-chain programs, then I think what we're missing is the full specification of each of these nodes, and the tree itself.

My main concern is that some of these node specifications may have more to do with Kinobi's ability to generate valid clients than to describe the program implementation.

Perhaps there's no difference, really? Maybe Kinobi does two things: defines a program's specification and then uses that specification to generate clients. The former being a highly reusable and composable utility beyond generated clients.

Anyway, just a little brain dump here. I think there's no doubt Kinobi is our best bet. But I want to hear Firedancer's opinion.

@lorisleiva
Copy link
Member Author

@buffalojoec Thank you for your detailed feedback.

I think what we're missing is the full specification of each of these nodes, and the tree itself.

By node/tree specification, do you mean things like documentation, JSON schemas, etc.? If so, that's definitely on my to-do list but yeah it's currently missing. 🫠

[node specs] may have more to do with Kinobi's ability to generate valid clients than to describe the program implementation.

I'm trying really hard to make sure there is no information on the tree that's only relevant to client generation or that wouldn't be relevant to apps like explorers that need to dynamically display as much info about a program as possible.

Before I actually used to have attributes like internal: boolean that would tell the renderers to render that node but not export it so devs could override them, etc. Now all of that has been stripped from the nodes themselves. If a renderer needs to have additional information about nodes, then they must accept special visitor options that's only relevant to them instead of polluting the tree.

That being said, since I'm pretty close to the problem I may have overlooked some attributes so please do let me know if there's anything you feel shouldn't belong to this majestic tree. ☺️

@buffalojoec
Copy link
Contributor

By node/tree specification, do you mean things like documentation, JSON schemas, etc.? If so, that's definitely on my to-do list but yeah it's currently missing. 🫠

Yes, and I totally figured it was on your list. 😅 No worries, just calling it out.

I'm trying really hard to make sure there is no information on the tree that's only relevant to client generation or that wouldn't be relevant to apps like explorers that need to dynamically display as much info about a program as possible.

I think we're aligned then! The use of the renderer configurations is the ideal middleman here, and I love that you went that route.

That being said, since I'm pretty close to the problem I may have overlooked some attributes so please do let me know if there's anything you feel shouldn't belong to this majestic tree. ☺️

Config is a smaller IDL with less AST nodes, and I think everything in here is totally agnostic. I think if we spot any nodes in the future than can be ported to renderer configs, let's just workshop that when the time comes?

@lorisleiva
Copy link
Member Author

That makes total sense! Let's do this.

@lorisleiva lorisleiva merged commit 396c398 into main Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants