Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
This is awesome. Thanks for getting this going! I'm still having a closer look at what you laid down on the SDK, but it appears to be heading in the right direction. In the meantime, have a look at this library, and let me know your thoughts on using it for interpreting the IDL into our Also, we should consider whether or not to use Anchor's client to do the heavy lifting on serialization, or fork it, make it more dynamic & generic, and possibly PR it back to the Coral team. |
buffalojoec
left a comment
There was a problem hiding this comment.
Love that you've started to lay this down, thank you.
I left a few comments in my review and I'll add a few more in here:
I'm not totally sold on whether or not I want the macro to write all of the TypeScript and Python code. Here's my thoughts, and I would love some input from you and anyone else:
- On one hand, I think it's pretty badass to get
.tsand.pyfiles generated on each build that are basically a representation of your program - just like an IDL - but in that specific programming context - On the other hand, I question whether or not this introduces more annoying dev-experience processes involving copy-pasting these files. I'm not sure this is an issue or not, but I've received this feedback
- Ultimately, Anchor is laying down an IDL and a TypeScript IDL context each time you build, and your client code either refers to the generated file directly or you copy-paste it into your source, so it's at least familiar
I guess the question is, should our client-side SDKs rely more heavily on the IDL JSON, or should the Rust code do most of the heavy lifting? Keeping in mind:
- We will have to update Rust token-string generation anytime these libraries change
- What if you want to change a dependency, and it continues to be overwritten every time you build?
- If we write too much into the generated
.tsand.py, we also approach an issue where the way the code is laid down by the Nautilus macro might not be how devs prefer to have it written. If they re-write any of it, they'll have to make those changes again every time their program changes. With this piece in mind, we should prob keep these files as simple as possible
Take a look at the TypeScript file Anchor lays down and notice how they're not involving any dependencies at all, ie. target/types/my_program.ts. It's all packed into a type and the actual types (like PublicKey) are written as strings. This might be a great concept to use for Nautilus as well, to avoid dependency chaos.
One of the true tests of a good SDK - in either JS or Python - is going to be replacing the manual client code I wrote in the main tests folder and instead plugging in both SDKs to interact with those test programs and their IDL/bindings.
This would be great to start doing as we introduce the client SDKs, but we should stash the manual code somewhere safe in the repo until the client SDKs are boiled out more.
Overall nice work, let's chat on some of the points I raised and hopefully get some other opinions in here! 🚀 🚀 🚀 🚀
js/tests/instantiate.test.ts
Outdated
| canInstantiate( | ||
| "string | no default program", | ||
| new Nautilus(CONNECTION, PROGRAM_ID_STRING), | ||
| "string | no programs", |
There was a problem hiding this comment.
this says "no programs" but you're passing in a program ID as a string still
There was a problem hiding this comment.
Some of the confusion in the tests comes from the fact that I struggled to understand what your intent for the constructor parameters were.
From my understanding, you could pass a list of programs (e.g. Token Metadata, Token2022, etc) that Nautilus will use, or take the default.
In this case, it says "no programs" because no program is overriden: there is no programs parameter passed.
js/src/index.ts
Outdated
| defaultProgram: PublicKey | undefined; | ||
| payer: Keypair | undefined; | ||
| programId: PublicKey; | ||
| programs?: {[programName: string]: PublicKey}; |
There was a problem hiding this comment.
Curious why you chose to do the string inside an array like that?
There was a problem hiding this comment.
This means that you can pass a dictionary where keys are strings, e.g: programs: { tokenMetadata: new PublicKey("XXXX") } would override the tokenMetadata program. I used this instead of an array of array ([PublicKey, string][]) to make it explicit what the key is vs what the value is.
js/src/index.ts
Outdated
|
|
||
| this.idl = idl | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any |
There was a problem hiding this comment.
I see why you need this annotation but I'm curious if we can use Object instead somehow?
There was a problem hiding this comment.
I will dig deeper into this, I used the annotation as a workaround but forgot to add a // TODO
js/src/index.ts
Outdated
| export class Nautilus { | ||
| import { decapitalizeFirstLetter } from './util'; | ||
|
|
||
| export class Nautilus<Program extends NautilusProgram = NautilusProgram> { |
There was a problem hiding this comment.
since a Nautilus instance is extending NautilusProgram, that would mean we are binding Nautilus instances 1-to-1 with programs, right?
This implementation is good and makes sense, but I can't shake the idea of - in the future - one Nautilus instance being able to operate on multiple programs, including non-Nautilus programs that have an IDL (Anchor, Shank)
There was a problem hiding this comment.
I see, that's what was meant by the multiple programs in the constructor!
IMO, one NautilusProgram <-> Nautilus instance is a good place to start because it's conceptually simpler, similar to Anchor (devs are familiar) and easier to implement.
I understand that being able to run join queries between programs would be awesome, but I feel like having a separate class handle that would still make sense, e.g. a NautilusCluster that takes separate NautilusPrograms as input.
| VersionedTransaction, | ||
| } from '@solana/web3.js'; | ||
|
|
||
| export class NautilusUtils { |
There was a problem hiding this comment.
Makes sense to not have these functions in a class for multiple reasons. Good stuff.
I wonder if we should try to make these private, ie. only exported to the Nautilus instance but not to anyone downstream?
There was a problem hiding this comment.
I feel like utility functions should be public because I used Anchor and Metaplex utils a lot
js/src/index.ts
Outdated
| payer?: Keypair | undefined; | ||
|
|
||
| util: NautilusUtils = new NautilusUtils(); | ||
| readonly idl: NautilusIdl; |
There was a problem hiding this comment.
This is cool, but I'm curious if we need to store the whole IDL in the instance, subsequently carrying it around anyone's client bundle?
There was a problem hiding this comment.
Probably not, I think it could work to only have it as a constructor argument. I'll work on that
|
Also take a look at the CI for Node JS, it's trying to run |
|
@Dodecahedr0x I'm finally getting some time to focus on the client SDK, so I'll be revisiting this PR as I lay a few things down ASAP. Let's roll |
|
@buffalojoec Sorry for taking so much time, I got sidetracked with other responsibilities. I pushed the last progress I did. There is still some work to at least finish types, before going into the functionalities I believe. I will keep working on it whenever I get enough bandwidth! |
|
I ended up going back to your idea of using multiple programs as input for a Nautilus instance. By passing an array of IDL, I get the tables of all programs and initialize them. However, there can still be collision between tables with the same name from different programs. |
This PR is an early draft of the TS SDK that addresses issue #21 . It aims to demonstrate how it could be done, using a generics system similar to what Anchor does.
Done:
To do: