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

refactor(experimental): add accounts package #1855

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

lorisleiva
Copy link
Contributor

@lorisleiva lorisleiva commented Nov 17, 2023

This PR adds a new @solana/accounts package that allows the new web3.js library to have its own definition of a Solana account as opposed to relying on the one provided by the RPC client that is limited to encoded accounts.

See README below for more details.


This package contains types and helper methods for representing, fetching and decoding Solana accounts. It can be used standalone, but it is also exported as part of the Solana JavaScript SDK @solana/web3.js@experimental.

It provides a unified definition of a Solana account regardless of how it was retrieved and can represent both encoded and decoded accounts. It also introduces the concept of a MaybeAccount which represents a fetched account that may or may not exist on-chain whilst keeping track of its address in both cases.

Helper functions are provided for fetching, parsing and decoding accounts as well as asserting that an account exists.

// Fetch.
const myAddress = address('1234..5678');
const myAccount = fetchAccount(rpc, myAddress);
myAccount satisfies MaybeEncodedAccount<'1234..5678'>;

// Assert.
assertAccountExists(myAccount);
myAccount satisfies EncodedAccount<'1234..5678'>;

// Decode.
type MyAccountData = { name: string; age: number };
const myDecoder: Decoder<MyAccountData> = getStructDecoder([
    ['name', getStringDecoder({ size: getU32Decoder() })],
    ['age', getU32Decoder()],
]);
const myDecodedAccount = decodeAccount(myAccount, myDecoder);
myDecodedAccount satisfies Account<MyAccountData, '1234..5678'>;

Types

BaseAccount

The BaseAccount type defines the attributes common to all Solana accounts. Namely, it contains everything stored on-chain except the account data itself.

const BaseAccount: BaseAccount = {
    executable: false,
    lamports: lamports(1_000_000_000n),
    programAddress: address('1111..1111'),
};

This package also exports a BASE_ACCOUNT_SIZE constant representing the size of the BaseAccount attributes in bytes.

const myTotalAccountSize = myAccountDataSize + BASE_ACCOUNT_SIZE;

Account and EncodedAccount

The Account type contains all the information relevant to a Solana account. It contains the BaseAccount described above as well as the account data and the address of the account.

The account data can be represented as either a Uint8Array — meaning the account is encoded — or a custom data type — meaning the account is decoded.

// Encoded.
const myEncodedAccount: Account<Uint8Array, '1234..5678'> = {
    address: address('1234..5678'),
    data: new Uint8Array([1, 2, 3]),
    executable: false,
    lamports: lamports(1_000_000_000n),
    programAddress: address('1111..1111'),
};

// Decoded.
type MyAccountData = { name: string; age: number };
const myDecodedAccount: Account<MyAccountData, '1234..5678'> = {
    address: address('1234..5678'),
    data: { name: 'Alice', age: 30 },
    executable: false,
    lamports: lamports(1_000_000_000n),
    programAddress: address('1111..1111'),
};

The EncodedAccount type can also be used to represent an encoded account and is equivalent to an Account with a Uint8Array account data.

myEncodedAccount satisfies EncodedAccount<'1234..5678'>;

MaybeAccount and MaybeEncodedAccount

The MaybeAccount type is a union type representing an account that may or may not exist on-chain. When the account exists, it is represented as an Account type with an additional exists attribute set to true. When it does not exist, it is represented by an object containing only the address of the account and an exists attribute set to false.

// Account exists.
const myExistingAccount: MaybeAccount<MyAccountData, '1234..5678'> = {
    exists: true,
    address: address('1234..5678'),
    data: { name: 'Alice', age: 30 },
    executable: false,
    lamports: lamports(1_000_000_000n),
    programAddress: address('1111..1111'),
};

// Account does not exist.
const myMissingAccount: MaybeAccount<MyAccountData, '8765..4321'> = {
    exists: false,
    address: address('8765..4321'),
};

Similarly to the Account type, the MaybeAccount type can be used to represent an encoded account by using the Uint8Array data type or by using the MaybeEncodedAccount helper type.

// Encoded account exists.
const myExistingAccount: MaybeEncodedAccount<'1234..5678'> = {
    exists: true,
    address: address('1234..5678'),
    data: new Uint8Array([1, 2, 3]),
    // ...
};

// Encoded account does not exist.
const myMissingAccount: MaybeEncodedAccount<'8765..4321'> = {
    exists: false,
    address: address('8765..4321'),
};

Functions

assertAccountExists()

Given a MaybeAccount, this function asserts that the account exists and allows it to be used as an Account type going forward.

const myAccount: MaybeEncodedAccount<'1234..5678'>;
assertAccountExists(myAccount);

// Now we can use myAccount as an Account.
myAccount satisfies EncodedAccount<'1234..5678'>;

parseBase64RpcAccount()

This function parses a base64-encoded account provided by the RPC client into an EncodedAccount type or a MaybeEncodedAccount type if the raw data can be set to null.

const myAddress = address('1234..5678');
const myRpcAccount = await rpc.getAccountInfo(myAddress, { encoding: 'base64' }).send();
const myAccount: MaybeEncodedAccount<'1234..5678'> = parseBase64RpcAccount(myRpcAccount);

parseBase58RpcAccount()

This function parses a base58-encoded account provided by the RPC client into an EncodedAccount type or a MaybeEncodedAccount type if the raw data can be set to null.

const myAddress = address('1234..5678');
const myRpcAccount = await rpc.getAccountInfo(myAddress, { encoding: 'base58' }).send();
const myAccount: MaybeEncodedAccount<'1234..5678'> = parseBase58RpcAccount(myRpcAccount);

parseJsonRpcAccount()

This function parses an arbitrary jsonParsed account provided by the RPC client into an Account type or a MaybeAccount type if the raw data can be set to null. The expected data type should be explicitly provided as the first type parameter.

const myAccount: Account<MyData> = parseJsonRpcAccount<MyData>(myJsonRpcAccount);

fetchEncodedAccount()

This function fetches a MaybeEncodedAccount from the provided RPC client and address. It uses the getAccountInfo RPC method under the hood with base64 encoding and an additional configuration object can be provided to customize the behavior of the RPC call.

const myAddress = address('1234..5678');
const myAccount: MaybeEncodedAccount<'1234..5678'> = await fetchEncodedAccount(rpc, myAddress);

// With custom configuration.
const myAccount: MaybeEncodedAccount<'1234..5678'> = await fetchEncodedAccount(rpc, myAddress, {
    abortSignal: myAbortController.signal,
    commitment: 'confirmed',
});

fetchEncodedAccounts()

This function fetches an array of MaybeEncodedAccount from the provided RPC client and an array of addresses. It uses the getMultipleAccounts RPC method under the hood with base64 encodings and an additional configuration object can be provided to customize the behavior of the RPC call.

const myAddressA = address('1234..5678');
const myAddressB = address('8765..4321');
const [myAccountA, myAccountB] = await fetchEncodedAccounts(rpc, [myAddressA, myAddressB]);
myAccountA satisfies MaybeEncodedAccount<'1234..5678'>;
myAccountB satisfies MaybeEncodedAccount<'8765..4321'>;

// With custom configuration.
const [myAccountA, myAccountB] = await fetchEncodedAccounts(rpc, [myAddressA, myAddressB], {
    abortSignal: myAbortController.signal,
    commitment: 'confirmed',
});

fetchJsonParsedAccount()

This function fetches a MaybeAccount from the provided RPC client and address by using getAccountInfo under the hood with the jsonParsed encoding. It may also return a MaybeEncodedAccount if the RPC client does not know how to parse the account at the requested address. In any case, the expected data type should be explicitly provided as the first type parameter.

type TokenData = { mint: Address; owner: Address };
const myAccount = await fetchJsonParsedAccount<TokenData>(rpc, myAddress);
myAccount satisfies MaybeAccount<TokenData> | MaybeEncodedAccount;

// With custom configuration.
const myAccount = await fetchJsonParsedAccount<TokenData>(rpc, myAddress, {
    abortSignal: myAbortController.signal,
    commitment: 'confirmed',
});

fetchJsonParsedAccounts()

Similarly to the fetchJsonParsedAccount method, this method fetches an array of MaybeAccount from a provided RPC client and an array of addresses. It uses the getMultipleAccounts RPC method under the hood with the jsonParsed encoding. It may also return a MaybeEncodedAccount instead of the expected MaybeAccount if the RPC client does not know how to parse some of the requested accounts. In any case, the array of expected data types should be explicitly provided as the first type parameter.

type TokenData = { mint: Address; owner: Address };
type MintData = { supply: bigint };
const [myAccountA, myAccountB] = await fetchJsonParsedAccounts<[TokenData, MintData]>(rpc, [myAddressA, myAddressB]);
myAccountA satisfies MaybeAccount<TokenData> | MaybeEncodedAccount;
myAccountB satisfies MaybeAccount<MintData> | MaybeEncodedAccount;

decodeAccount()

This function transforms an EncodedAccount into an Account (or a MaybeEncodedAccount into a MaybeAccount) by decoding the account data using the provided Decoder instance.

type MyAccountData = { name: string; age: number };

const myAccount: EncodedAccount<'1234..5678'>;
const myDecoder: Decoder<MyAccountData> = getStructDecoder([
    ['name', getStringDecoder({ size: getU32Decoder() })],
    ['age', getU32Decoder()],
]);

const myDecodedAccount = decodeAccount(myAccount, myDecoder);
myDecodedAccount satisfies Account<MyAccountData, '1234..5678'>;

@mergify mergify bot added the community label Nov 17, 2023
@mergify mergify bot requested a review from a team November 17, 2023 10:35
@lorisleiva lorisleiva self-assigned this Nov 17, 2023
@buffalojoec
Copy link
Contributor

Overall this API looks well-organized from the documentation, but I didn't review the code yet. I was hoping to discuss the intention(s) a bit more first before reviewing.

First and foremost, my knee-jerk reaction here is that this seems like a lot of boilerplate for what I can pretty easily do with getAccountInfo and getStructCodec().

// Decode.
type MyAccountData = { name: string; age: number };
const myDecoder: Decoder<MyAccountData> = getStructDecoder([
  ["name", getStringDecoder({ size: getU32Decoder() })],
  ["age", getU32Decoder()],
]);
const myDecodedAccount = decodeAccount(myAccount, myDecoder);

This is exactly the same as this:

type MyAccountData = { name: string; age: number };
const myDecoder: Decoder<MyAccountData> = getStructDecoder([
  ["name", getStringDecoder({ size: getU32Decoder() })],
  ["age", getU32Decoder()],
]);
const myDecodedAccount = myDecoder.decode(myAccount.data);

Similarly, fetchEncodedAccount and fetchEncodedAccounts map to getAccountInfo and getMultipleAccounts, respectively, with almost no difference besides the "maybe" type returned.

Speaking of which, the whole concept of MaybeAccount and MaybeEncodedAccount seems highly gratuitous. The RPC method already returns null for accounts that don't exist, so you've already got a specific type for an account that doesn't exist out of the box (null). Furthermore, you just hit the method with the desired account's address, so you have that, too. I don't see how two "maybe" types with an exists boolean makes this any easier/smoother.

Sorry to be so negative here, but I'm struggling to see exactly why we need all of this.

On the positive side, I dig Account and EncodedAccount. I think the implementation exactly as you've documented it is pretty darn good:

// Encoded.
const myEncodedAccount: Account<Uint8Array, "1234..5678"> = {
  address: address("1234..5678"),
  data: new Uint8Array([1, 2, 3]),
  executable: false,
  lamports: lamports(1_000_000_000n),
  programAddress: address("1111..1111"),
  rentEpoch: 42n,
};

// Decoded.
type MyAccountData = { name: string; age: number };
const myDecodedAccount: Account<MyAccountData, "1234..5678"> = {
  address: address("1234..5678"),
  data: { name: "Alice", age: 30 },
  executable: false,
  lamports: lamports(1_000_000_000n),
  programAddress: address("1111..1111"),
  rentEpoch: 42n,
};

Although I have to say I think including the address in the type signature might be overkill as well. ie:

let myEncodedAccount: Account<Uint8Array, "1234..5678">;
let myEncodedAccount: Account<Uint8Array>;

It's worth mentioning that another positive here is the legacy AccountInfo type was generic over some type, with the encoded version using Buffer. So, this won't be unfamiliar to people, which is good.

In summary I think we can still add some nice dressing to the library as it relates to working with accounts with about half of this stuff or less.

@lorisleiva
Copy link
Contributor Author

lorisleiva commented Nov 20, 2023

Thanks for the detailed review @buffalojoec! ❤️

Overall, I agree that this library is a light abstraction layer over the current solution — i.e. getAccountInfo, getMultipleAccount, Some RPC-specific type for encoded accounts and null for missing accounts.

It seems we at least both agree on the following premise: There needs to be an account definition that's distinct from the one tight to the JSON RPC spec. Why? Because:

  • The RPC may return multiple versions of encoded accounts,
  • It doesn't include the address of the account,
  • It doesn't support decoded account data (aside from a mediocre parsed JSON attempt for a few recognised programs),
  • And, as an added bonus, we have control over its account header (e.g. we can call owner, programAddress to stay consistent with the rest of the library although that deserves its own debate lol).

Now, let's look into the other parts of the API. I'll give you more details as to why I've added them and if you still think they are overkill, I'm more than happy to trim it down. I just want you to get the full picture before making that decision.

Maybe Accounts

My reason for introducing that "Schrödinger's account" concept is purely because, when building dApp, it's a pain to not know the address of the missing accounts.

You're receiving an array of type Array<Account | null> and you're having to match its indices with the provided array of addresses.

const myAccountsOrNull = await rpc.getMultipleAccounts(myAddresses);
const addressesOfMissingAccounts = myAddresses.filter((address, index) => myAccountsOrNull[index] === null);

Whereas I think it's more intuitive to do:

const myMaybeAccounts = await fetchEncodedAccounts(rpc, myAddresses);
const addressesOfMissingAccounts = myMaybeAccounts
  .filter(account => !account.exists)
  .map(account => account.address)

That being said, I can understand that, the cost of adding and supporting this helper type is maybe not worth the benefit it provides.

Decoder helper

Here as well, I agree that the decoder helper is maybe a bit overkill. Although if we go for the API change proposed in #1865, it would fit nicely with the decode(bytes, decoder) pattern it provides. I do love a consistent API.

Also, your before/after comparison is not completely correct:

// With the decode helper.
const myDecodedAccount = decodeAccount(myAccount, myDecoder);

// Without the decode helper.
const myDecodedAccount = {
  ...myAccount,
  data: myDecoder.decode(myAccount.data)[0]
};

Fetch helpers

Here, I think these helpers are important.

Even if we don't greatly change the structure of the Account we own from the raw account we get from the RPC, we do need functions that enables us to go from one to the other.

Even if we discard the fetch helpers and export the parse helpers instead, we end up exposing two account definition to the end-user which is not ideal:

// With parsing helpers only.
const myRpcAccount = await rpc.getAccountInfo(myAddress);
const myAccount = parseEncodedAccount(myAddress, myRpcAccount);

// With fetching helpers that hide the parsing from the end-user.
const myAccount = await fetchEncodedAccount(rpc, myAddress);

And it's even worse with getMultipleAccounts as you have to zip the addresses with the accounts received.

In short, these helpers are the abstraction layer between the raw RPC account info and the one we want to maintain in this library.

Address type param

This library consistently offer a TAddress type parameter for any type that contains a base58 encoded address (PDAs, signers, account metas, program addresses, etc). I fail to see why this should not be the case here.

@buffalojoec
Copy link
Contributor

Well that's exactly the context I needed. As expected, well thought-out approaches and arguments. Some of these use cases on the dApp side weren't apparent to me.

I think we're now mostly in agreement, then. Using the fetch functions to wrap the RPC and go from the response object to our new Account types is valid. I also forgot that the data field coming back from the new RPC API is going to be a string, not UInt8Array, which means your decoder skips at least two steps not one, which is also valid.

My reason for introducing that "Schrödinger's account" concept is purely because, when building dApp, it's a pain to not know the address of the missing accounts.

You're receiving an array of type Array<Account | null> and you're having to match its indices with the provided array of addresses.

As a standalone call it doesn't really make a lot of sense, but considered across getMultipleAccounts it definitely does. I feel like this puts us in a weird spot where fetchEncodedAccount(..): MaybeAccount starts to appear gratuitous, but at the behest of fetchEncodedAccounts(..): : MaybeAccount[] being quite useful over its RPC counterpart.

I'm sure you put plenty of thought into that, though, so whatever you think is best on this part I'm good with.

Side note: what's the TypeScript annoyingness-rating of something like this:

// MaybeEncodedAccount[]
maybeEncodedAccounts.map(
  if (account.exists) {
    <ExistingAccountComponent lamports={lamports} data={data} />
  } else {
    <NullAccountComponent address={address} />
  }
)
  • And, as an added bonus, we have control over its account header (e.g. we can call owner, programAddress to stay consistent with the rest of the library although that deserves its own debate lol).

I went with ownerProgram in GraphQL. Would love to have parity there! Whatever we decide I'll change it to, though.

@lorisleiva
Copy link
Contributor Author

Nice, I'm glad this helped add some context to the PR! 🙌

If I understand correctly we're left with two subjects to make a decision on: Maybe accounts and the replacement name of account.owner.

Maybe accounts

I'm not too bothered about that one to be honest. I can see MaybeAccounts bring value but I'm not 100% sure it's valuable enough to justify adding it. I'd like @steveluscher and @mcintyre94's opinions on that one too.

The pros are:

  • Address stored on missing accounts makes it easier to deal with them: no need to match the addresses input with the null values of the response.
  • Consistent API with Account since both of them store the account's address (as opposed to the RPC's account representation).
  • Useful for fetching multiple accounts.

The cons are:

  • Overhead of adding, documenting and maintaining in extra type.
  • Overkill for fetching single accounts.

Side note: what's the TypeScript annoyingness-rating of something like this:

The equivalent of not using MaybeAccounts though would be more messy IMO:

// (Account | null)[] created from an array of `addresses`.
accountsOrNull.map((account, index) => {
  if (account) {
    <ExistingAccountComponent lamports={account.lamports} data={account.data} />
  } else {
    <NullAccountComponent address={addresses[index]} />
  }
})

Naming account.owner

ownerProgram is a nice name too. I agree we should stay consistent though. The reason I went with programAddress is because that's what we currently use for instructions and PDA derivations.

Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

Shit. I had a bunch of comments last week and forgot to commit them.

packages/accounts/src/__tests__/fetch-accounts-test.ts Outdated Show resolved Hide resolved
packages/accounts/src/account-header.ts Outdated Show resolved Hide resolved
packages/accounts/src/account-header.ts Outdated Show resolved Hide resolved
@steveluscher
Copy link
Collaborator

I can see MaybeAccounts bring value but I'm not 100% sure it's valuable enough to justify adding it.

https://discord.com/channels/428295358100013066/1072928238076182589/1176212465441312830

@mcintyre94
Copy link
Contributor

I think my concern with this API would be the lack of support for jsonParsed accounts.

If I want to fetch say a token account, my best bet would be to use getAccountInfo with jsonParsed and then I'll get the nicely parsed data for that account in the RPC response

Whereas if I want to fetch some other program account I'd want to use the fetchEncodedAccount and decodeAccount functions here instead

I think it's potentially a bit confusing to have the mix of lower level getAccountInfo calls with these calls, but I'm not sure what the right solution is for that in this API. Most accounts don't have jsonParsed RPC support, but when it is there it's the best option

@steveluscher
Copy link
Collaborator

I'm just here to cast a soft-vote in the direction of ‘every abstraction has a maintenance cost but some deliver more value than others.’

@lorisleiva
Copy link
Contributor Author

@mcintyre94 That's a great point that I didn't get to touch on in the description of this PR so thanks for bringing that up.

Where serialisation should live on Solana is a big question. One that we still don't have a clear answer for. The contenders for this question are:

  • On the client. The program defines a set of types, accounts and instruction data which is then used to generate a library that encodes and decodes these components. This includes any serialisation definition of these components: IDLs, Runtime V2, etc. This is how Kinobi works. It accepts any definition of a program, standardise it as a tree of nodes and visit these nodes to render code such as a JS client.
  • On the RPC. Here we push the serialisation schemas to the RPC somehow such that when sending RPC methods such as getAccountInfo, it is able to return the decoded data directly. There are lots of conversations around that these days (especially with RPC v2 talks) but in my opinion, our current serialisation schema is too weak to make that happen. IDLs are tailored for Borsh serialisation which is often not performant enough for programs. Until we come up with a "Serialization of Everything" schema, or a programmatic way to provide that information to RPCs, this is not a fully encompassing solution.

Right now, RPCs offer a little bit of both where most of the time you'd get encoded account data but every now and then you'd be able to get the decoded data depending if that specific program is supported by your specific RPC. This inconsistency is not something I'd like to bring over to this API.

Let's fully embrace the first contender, and provide a consistent API, regardless of the program you're requesting and the RPC you're using.

What does this mean concretely for accounts that can be parsed by RPCs like token accounts? Well, it means we'll provide fetching and decoding helpers just like we would for any program.

const myTokenAccount: Token = await fetchToken(rpc, myTokenAddress);
const myMintAccount: Mint = await fetchMint(rpc, myMintAddress);
const myCustomDummyAccount: CustomDummy = await fetchCustomDummy(rpc, myCustomDummyAddress);

@mcintyre94
Copy link
Contributor

@lorisleiva I think that's actually helped me understand how this library fits in a bit more. Is the idea here that you want to generate code like fetchMyThing(rpc, thingAddress) for a program, and that would use fetchEncodedAccount + a generated decoder?

@lorisleiva
Copy link
Contributor Author

@mcintyre94 Yes, that's right! For each account, I generate helpers like decodeThing which would use decodeAccount under the hood and fetchThing or fetchAllThing which would use (fetchEncodedAccount or fetchEncodedAccounts) and decodeThing under the hood.

@buffalojoec
Copy link
Contributor

@lorisleiva @mcintyre94 If we wanted to support it, but maybe not encourage it, we could roll a getJsonParsedCodec(). Maybe it doesn't even have to be a codec, since it's not decoding anything, but rather transforming an already decoded object.

Regardless, this would allow us to use parseEncodedAccount(..) or a similar method for jsonParsed encoding to go from GetAccountInfoResponseBase & AccountDataJsonParsed (or whatever) to Account<Mint>.

With this approach, you get all of the niceties of Loris' API without doing all the decoding on your app. Maybe you're doing some large operation where it makes sense to put the parsing load (especially for tokens under SPL) on the server, rather than your client. In this scenario, jsonParsed as an RPC request config is the obvious choice.

Note jsonParsed follows the same pattern for accounts and instructions, so this only needs to be implemented once.

@lorisleiva
Copy link
Contributor Author

@buffalojoec The issue is, even for the small subset of programs it supports, the jsonParsed data won't exactly match the data we generate (e.g. see the UI token amounts 🤢).

So we'll end up having to not only write parser code for each of these programs but also make sure we keep updating that list of parser when more programs are added to that list. IMO this jsonParsed data type is a stop gap until we figure out a way to support RPC-side serialisation.

Here again it's a case of, would supporting jsonParsed data bring more value than it would cost to maintain? Considering the workaround is simply to use the rpc methods directly, I'd say no.

@lorisleiva
Copy link
Contributor Author

@buffalojoec @mcintyre94 I just had a nice chat with @joncinque who gave me permission to quote him on that. Jon said the jsonParsed API was a mistake and, whilst we'll have to continue maintaining it, it won't be the recommended way to fetch data in the future.

Regarding performances, it seems RPCs don't even store the decoded data and only the token owner is indexed (because it was needed for the Solana Explorer at the time). Therefore, by using jsonParsed, we're not even making our app more performant, we're just pushing the workload to the RPC.

I'd very much like to cast my vote for "let's deprecate this thing". 😄

@steveluscher
Copy link
Collaborator

Therefore, by using jsonParsed, we're not even making our app more performant, we're just pushing the workload to the RPC.

Except by pushing the parsing logic to the RPC (or whatever server layer) you don't have to ship every single parser, as JavaScript, to the client, which shrinks bundle sizes and makes websites load faster.

86uws8

@lorisleiva
Copy link
Contributor Author

@steveluscher Until we have a proper solution for RPC-side serialisation, we're gonna have to ship these decoders for 99% of the programs anyway. Let's make a consistent Client-side serialisation API instead please. 🙏 See my comment here.

@lorisleiva lorisleiva force-pushed the loris/accounts-package branch from 3874e38 to 13d22dd Compare November 22, 2023 11:31
@lorisleiva
Copy link
Contributor Author

We've had lots of nice discussions here but I'm a bit lost now when it comes to figuring out the next steps for this to be merged.

I believe this is a good API that's going to elevate code gen. One that's close to how Umi handles accounts which has had lots of positive feedback.

Is there anything anyone is completely against in this PR?

Cc @buffalojoec @steveluscher @mcintyre94

@mcintyre94
Copy link
Contributor

To be completely honest I don't think I have a good feeling for what we're trying to do with the higher-level API so it's a bit tricky to give useful feedback.

I think this is a good API to expose to developers, though I don't think getAccountInfo + decode yourself is too bad. I can see why you want more structure for code gen though, and it makes more sense to put that in a library like this than only in generated code.

Overall I'm in favour 👍

@lorisleiva
Copy link
Contributor Author

Thanks Callum, to give you more context this library will help me generate the following types and functions for a given program account:

// Taking the Metadata account from Metaplex as an example.

type Metadata<TAddress> = Account<MetadataAccountData, TAddress>;
type MetadataAccountData = { ... };
type MetadataAccountDataArgs = { ... };

function getMetadataAccountDataEncoder(): Encoder<MetadataAccountDataArgs> { ... }
function getMetadataAccountDataDecoder(): Decoder<MetadataAccountData> { ... }
function getMetadataAccountDataCodec(): Codec<MetadataAccountDataArgs, MetadataAccountData> { ... }
function decodeMetadata<TAddress>(encodedAccount: EncodedAccount<TAddress>): Metadata<TAddress> { ... }

async function fetchMetadata<TAddress>(context, address: Base58EncodedAddress<TAddress>, options): Promise<Metadata<TAddress>> { ... }
async function safeFetchMetadata<TAddress>(context, address: Base58EncodedAddress<TAddress>, options): Promise<Metadata<TAddress> | null> { ... }
async function fetchAllMetadata(context, addresses: Base58EncodedAddress[], options): Promise<Metadata[]> { ... }
async function safeFetchAllMetadata(context, addresses: Base58EncodedAddress[], options): Promise<Metadata[]> { ... }

// For fixed-size accounts only:
function getMetadataSize(): number { ... }

// For PDAs only:
async function findMetadataPda(context, seeds): Promise<ProgramDerivedAddress> { ... }
async function fetchMetadataFromSeeds(context, seeds, options): Promise<Metadata> { ... }
async function safeFetchMetadataFromSeeds(context, seeds, options): Promise<Metadata | null> { ... }

The thing I can live without for this code gen though is the MaybeAccount type. If you feel it doesn't add much value, I'm happy to replace it with null.

@buffalojoec
Copy link
Contributor

We've had lots of nice discussions here but I'm a bit lost now when it comes to figuring out the next steps for this to be merged.

I believe this is a good API that's going to elevate code gen. One that's close to how Umi handles accounts which has had lots of positive feedback.

Is there anything anyone is completely against in this PR?

Cc @buffalojoec @steveluscher @mcintyre94

I guess I'm on the fence for MaybeAccount and I'm thinking scrapping jsonParsed support is a potential issue. The rest of the API I'm good with!

I understand adding conversions for jsonParsed and ensuring compatibility with the generated types will be super annoying, but I personally think we have to include it in some capacity.

Anyone who wants to use this accounts API and has god knows how large of an application built on jsonParsed response types is going to have to roll their own conversions anyway, since any developer is going to think to themself, "why would I go from getting parsed accounts for free to decoding encoded accounts on my app?", and that question becomes much more relevant as the query count/size increases.

Regarding performances, it seems RPCs don't even store the decoded data and only the token owner is indexed (because it was needed for the Solana Explorer at the time). Therefore, by using jsonParsed, we're not even making our app more performant, we're just pushing the workload to the RPC.

I won't argue to what specific magnitude moving all decoding to the client will affect bundle size and app performance, but there's plenty of people in the ecosystem who definitely will.

I'm also starting to think people might get confused by the wording of EncodedAccount, considering encoding is used on the RPC for 4 other account encoding types that aren't raw bytes. 💀 💀

Sorry to continue to be vague, but I don't have a specific solution in mind yet. I'm stewing on:

A: Manually roll converters from jsonParsed to T.
B: Replace jsonParsed schemas in the RPC server codebase with JSON Abstract-Syntax Trees and use them in this library, too

B is dangerous for RPC v1, but it can definitely work in v2.

@lorisleiva
Copy link
Contributor Author

Thanks Joe, I'm also on the fence with MaybeAccounts and I might just remove them at this point.

Regarding jsonParsed, this is where I strongly disagree. 😬

First of all, the helper method is called fetchEncodedAccount which makes it clear we're only looking to get encoded data and not parsed one. It also force the use of base64 encoding meaning there shouldn't be the encoding option ambiguity you mentioned (which I now realise I probably need to Omit from the relevant TypeScript type).

Now, to tackle your proposed solutions.

A) requires us to provide custom parsing logic for each supported program which will end up making your bundle size even bigger! Decoding on the new web3.js always rely on the same set of codecs being imported (struct, address, u32, etc.) so the likelihood of you already having these codecs on your app is very high. Not to mention you'll need them anyway to encode and decode your transactions. Not only that, but it will couple the new web3.js with program logic which would violate one of our core principles: "Program wrappers belong to their own packages".

B) is essentially implementing RPC-side serialisation which is far from being ready.

@buffalojoec
Copy link
Contributor

A) requires us to provide custom parsing logic for each supported program which will end up making your bundle size even bigger! Decoding on the new web3.js always rely on the same set of codecs being imported (struct, address, u32, etc.) so the likelihood of you already having these codecs on your app is very high. Not to mention you'll need them anyway to encode and decode your transactions. Not only that, but it will couple the new web3.js with program logic which would violate one of our core principles: "Program wrappers belong to their own packages".

What about creating one single JsonParsedDecoder that takes the exact same arguments as getStructDecoder() but applies that provided map to the info field of a jsonParsed payload? It could also take a StructDecoder as well, making it easy to plug in the struct decoders from those generated program client libraries.

Because jsonParsed is the same across accounts and instructions, and we don't want unnecessary bundle size increase, maybe we keep this decoder separate somehow? Not coupled to @solana/accounts or @solana/instructions or any particular client.

I'm envisioning something like this for flow:

type MyAccountData = { name: string; age: number };

const myAccount: JsonParsedAccount<"1234..5678">;
// { parsed: { info: { name: 'joe'; age: 4 }, type: 'foo' }, ... }

const myDecoder: Decoder<MyAccountData> = getJsonParsedDecoder([
  ["name", getStringDecoder({ size: getU32Decoder() })],
  ["age", getU32Decoder()],
]);

const myDecodedAccount = decodeAccount(myAccount, myDecoder);
myDecodedAccount satisfies Account<MyAccountData, "1234..5678">;

You could also follow similar patterns for MaybeJsonParsedAccount and assertJsonParsedAccount.

One drawback that jumps out at me is there's no way to guarantee the type response from the RPC as jsonParsed, like there is for base64... 🫠

Copy link

socket-security bot commented Dec 1, 2023

No top level dependency changes detected. Learn more about Socket for GitHub ↗︎

@lorisleiva lorisleiva marked this pull request as draft December 1, 2023 14:30
@lorisleiva lorisleiva force-pushed the loris/refactor-codecs-in-other-packages branch from e1e5b32 to f46a9d7 Compare December 1, 2023 15:26
Base automatically changed from loris/refactor-codecs-in-other-packages to master December 1, 2023 15:27
@lorisleiva lorisleiva force-pushed the loris/accounts-package branch 7 times, most recently from bc80020 to b76d13f Compare December 1, 2023 20:41
@lorisleiva lorisleiva marked this pull request as ready for review December 1, 2023 20:41
@lorisleiva
Copy link
Contributor Author

Alright, this is ready for another review. Significant changes are:

  • Repurposed parseEncodedAccount as parseRpcAccount as it now parses both encoded RPC account (base58 and base64) as well as jsonParsed account as long as you explicitly provide the data type as a type parameter.
  • Repurposed fetchEncodedAccount as fetchAccount as it now also supports fetching jsonParsed account.
  • Removed the rentEpoch attribute from the AccountHeader type.
  • Updated the README and tests.

Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

giphy.gif

Comment on lines 43 to 47
if (typeof rpcAccount.data === 'string') return getBase58Encoder().encode(rpcAccount.data);
if (typeof rpcAccount.data === 'object' && 'parsed' in rpcAccount.data)
return rpcAccount.data.parsed.info as TData;
if (rpcAccount.data[1] === 'base58') return getBase58Encoder().encode(rpcAccount.data[0]);
if (rpcAccount.data[1] === 'base64') return getBase64Encoder().encode(rpcAccount.data[0]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

So a few things.

  1. There's nothing really ‘rpc-y’ about account data. The only RPC-ness here is the shape of the account (ie. the fact that the data is at account.data[1]). It would make this thing more general purpose if the ‘parseRpcAccount’ and parseAccountData methods were separate, and the former made use of the latter. Another option is to eliminate parseRpcAccount altogether and make people index into their own data. Another option is to create parseBase64RpcAccount and parseBase58RpcAccount et cetera.
  2. The way that this function is written hard links both the base58 and base64 encoder implementations. This penalizes the person who only ever ‘decodes’ jsonParsed accounts. If you end up doing step 1 (or eliminating parseRpcAccount altogether) then you can make parseBase58AccountData, parseBase64AccountData, et cetera methods so the unused ones can get DCE'd.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting points raised. Commenting here to follow discussion.

Copy link
Contributor Author

@lorisleiva lorisleiva Dec 6, 2023

Choose a reason for hiding this comment

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

Great points.

  1. The RPC naming was simply to signal we are going from an account definition provided by the JSON RPC spec to an account definition provided by the library.
  2. You're 100% right, I didn't think about the treeshake optimisation we could do here.

As a result I did the following:

  • Split parseRpcAccount into 3 distinct functions: parseBase64RpcAccount, parseBase58RpcAccount and parseJsonRpcAccount.
  • Created a private parseBaseAccount function that each of these use under the hood.
  • Split fetchAccount into fetchEncodedAccount (which uses base64 encoding) and fetchJsonParsedAccount (which uses jsonParsed encoding).
  • Similarly, split fetchAccounts into fetchEncodedAccounts and fetchJsonParsedAccounts.
  • Updated the tests and README accordingly.

Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're making the distinction from encoded, should we also have a MaybeJsonParsedAccount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JSON parsed accounts can be returned as MaybeAccount<MyData> directly. Do you mean that new type would be the union of this and MaybeEncodedAccount?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhh my b, I think I was confusing it with decode. Lgtm.

packages/accounts/src/fetch-account.ts Outdated Show resolved Hide resolved
packages/accounts/src/__tests__/fetch-accounts-test.ts Outdated Show resolved Hide resolved
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.

Lgtm @lorisleiva!!

I left two pointers to ongoing discussions I think should land before this goes in.

packages/accounts/src/account-header.ts Outdated Show resolved Hide resolved
packages/accounts/src/fetch-account.ts Outdated Show resolved Hide resolved
Comment on lines 43 to 47
if (typeof rpcAccount.data === 'string') return getBase58Encoder().encode(rpcAccount.data);
if (typeof rpcAccount.data === 'object' && 'parsed' in rpcAccount.data)
return rpcAccount.data.parsed.info as TData;
if (rpcAccount.data[1] === 'base58') return getBase58Encoder().encode(rpcAccount.data[0]);
if (rpcAccount.data[1] === 'base64') return getBase64Encoder().encode(rpcAccount.data[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting points raised. Commenting here to follow discussion.

@lorisleiva lorisleiva force-pushed the loris/accounts-package branch 3 times, most recently from 9482bee to 9ae24d5 Compare December 6, 2023 14:55
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.

Thanks for all the committed revisions!

packages/accounts/src/account.ts Show resolved Hide resolved
@lorisleiva lorisleiva merged commit e1ca396 into master Dec 7, 2023
11 checks passed
@lorisleiva lorisleiva deleted the loris/accounts-package branch December 7, 2023 11:10
Copy link
Contributor

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants