verze-app / solana-php-sdk

Simple PHP SDK for Solana JSON RPC endpoints
MIT License
88 stars 45 forks source link

Port of Javascript Solana library #9

Closed exzachlyvv closed 2 years ago

exzachlyvv commented 2 years ago

This PR contains a majority of the core Solana API classes and interactions. It leverages PHP's sodium library for all things related to cryptography (TY sodium!).

I have done enough to submit real transactions successfully to the Solana blockchain (see SolanaRpcClientTest@it_test_zvv_real_transaction). Some areas have not been tested thoroughly yet.

Most of my reference point was the 1st party Javascript library, but also quite a bit from the community python library and a little from the community java library.

Some helpful things to know:

  1. In other languages, binary data is typically an array of bytes. In PHP, binary is typically a string (b"asdf"). There are 2 nice helpers in this PR, Ed25519Keypair::array2bin() and Ed25519Keypair::bin2array()

TODO probably in a different PR (in no particular order):

  1. Abstract away the binary implications of TransactionInstruction creation (see SystemProgram@transfer()). I hope developers do not need to be concerned with anything binary-related when using this package.
  2. Now that I better understand many of the moving parts, I'm sure there are many improvements to make.
  3. Build out more of the native API calls in the Connection class.
  4. Build out SystemProgram, SlpTokenProgram, StakeProgram (?), MetaplexProgram (?)
  5. Add linting/cs-fixer.
  6. Specific Exceptions. Right now I use the GenericException all over the place
  7. code coverage + unit test missing areas.
mattstauffer commented 2 years ago

Wow! So much work--I'm super impressed! Mind if I do some review and suggest some code style changes, purely personal preference stuff?

exzachlyvv commented 2 years ago

Please do, go crazy 😄

exzachlyvv commented 2 years ago

OK, so.. I got a third of the way into this review and it's clear to me you have a much greater capacity and knowledge and amount of time to work on this than me.

I want to fully offer to drop the Tighten version of this package and let you take it over under your namespace. Let me know what you think!

  1. Don't get discouraged! It all seems pretty complex without context, more complex than it really is. I didn't know too much when I began porting this and death-by-fire, here we are. Although I'm happy to lead this charge 💪
  2. To the note about moving this from Tighten to my namespace, it does not matter to me. I'm happy to take over if you would prefer. I'm happy to keep this right here if you want, too. I'll leave that up to you. I wonder if you think the Tighten namespace helps add some validity within the PHP+Laravel community that this will be well taken care of 🤔
exzachlyvv commented 2 years ago

From a functional standpoint, I am now ready to merge this.

mattstauffer commented 2 years ago

@exzachlyvv Thank you for your kind words!

And yah, if you'd rather be the maintainer but under the Tighten namespace, that's fine by me! Let me give you maintain permissions right now :)

exzachlyvv commented 2 years ago

@mattstauffer how are you feeling about this PR? Did you want more time to review the rest of the code? No rush, just don't want this getting stale.

Other things we should add before a v1.0 (I'll follow up with separate PR's):

  1. Buffer class to abstract away binary - this will probably result in breaking changes to the API.
  2. Build out more of the Connection / SystemProgram classes.
  3. Find the best way to abstract the binary implications of TransactionInstruction creation and binary implications in general. For example:
    $data = [
    // uint32
    ...unpack("C*", pack("V", self::PROGRAM_INDEX_CREATE_ACCOUNT)),
    // int64
    ...unpack("C*", pack("P", $lamports)),
    // int64
    ...unpack("C*", pack("P", $space)),
    //
    ...$programId->toBytes(),
    ];

    is not a very friendly API.

  4. Update readme.
mattstauffer commented 2 years ago

@exzachlyvv I think that I'm gonna try to take another look at it today. And then, whether or not i can review the whole thing, let's just merge.

That merge conflict hopefully will be easy to resolve; we can just re-delete the Solana class.

mattstauffer commented 2 years ago

Great work! I'm gonna tag this as a new dot release so I can keep my old apps on the original until I update.