Skip to content
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

[Draft] TS SDK #45

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

Dodecahedr0x
Copy link
Contributor

@Dodecahedr0x Dodecahedr0x commented May 8, 2023

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:

  • Expand the TS generation when building the IDL to include a type that can be used as generic when creating a Nautilus Instance
  • Start the work to use the generic type in the Nautilus instance
  • Refactored Nautilus constructor to make futur changes less breaking

To do:

  • Use data from the IDL and the generic type to display the correct interface
  • Expand SDK functionalities

@vercel
Copy link

vercel bot commented May 8, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nautilus ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 31, 2023 9:08pm

@buffalojoec
Copy link
Contributor

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 Program type: https://github.com/solana-idl-foundation/solana-idl-spec

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 buffalojoec self-requested a review May 9, 2023 22:29
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.

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 .ts and .py files 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 .ts and .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! 🚀 🚀 🚀 🚀

@@ -12,83 +14,23 @@ export function tests() {
}

canInstantiate(
"string | no default program",
new Nautilus(CONNECTION, PROGRAM_ID_STRING),
"string | no programs",
Copy link
Contributor

Choose a reason for hiding this comment

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

this says "no programs" but you're passing in a program ID as a string still

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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};
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why you chose to do the string inside an array like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

I see why you need this annotation but I'm curious if we can use Object instead somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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> {
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@@ -11,113 +11,118 @@ import {
VersionedTransaction,
} from '@solana/web3.js';

export class NautilusUtils {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like utility functions should be public because I used Anchor and Metaplex utils a lot

js/src/index.ts Outdated

util: NautilusUtils = new NautilusUtils();
readonly idl: NautilusIdl;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not, I think it could work to only have it as a constructor argument. I'll work on that

@buffalojoec
Copy link
Contributor

Also take a look at the CI for Node JS, it's trying to run npm ci on the bundle. Try to debug that locally and get the bundle building, and if we need to change the CI for what we're doing let me know

@buffalojoec
Copy link
Contributor

@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

@Dodecahedr0x
Copy link
Contributor Author

@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!

@Dodecahedr0x
Copy link
Contributor Author

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.
I also need to fix the query.ts file and how the Nautilus instance is used when parsing queries

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.

None yet

2 participants