-
Notifications
You must be signed in to change notification settings - Fork 48
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
feat: add js support for arbitrary state trees #1490
Conversation
6227bb6
to
0a30fb6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Added a couple comments, not sure that I understood the design completely.
I think there are some low hanging fruit to make the design future proof and minimize breaking changes in future updates.
In the forester and test indexer we handle multiple trees already with structs similar to TreePubkeys
.
See forester utils (note that we don't need cpi context in the forester but in clients for programs that escrow compressed tokens and handle another compressed account.)
activeStateTrees: PublicKey[]; | ||
activeQueues: PublicKey[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since state trees always have a queue I think it makes sense to store these in a single object to avoid nasty bugs of pairing a tree with the wrong queue whether by us or an external dev.
(Note BatchedAddressTrees don't have an additional queue account Merkle tree and address queue are the one account.)
export type TreeType {
State,
Address,
BatchedState,
BatchedAddress,
}
export type TreePubkeys {
tree: PublicKey,
// BatchedAddress have no queue
queue: PublicKey | undefined,
cpi_context: Pubkey | undefined,
tree_type: TreeType,
}
export type ActiveStateTreeAddresses = {
activeStateTrees: StateTreePubkeys[],
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another advantage of grouping pubkeys per tree is that many devs don't need to be aware that a tree even has a queue they just pass the TreePubkeys object.
authority: Keypair; | ||
}): Promise<{ txId: string }> { | ||
// to be nullified address must be part of stateTreeLookupTable set | ||
const stateTreeLookupTable = await connection.getAddressLookupTable( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the design with the lookup table.
We could introduce further conventions such as:
- that a lookuptable is mutable signals that it is the current one
- if a lookuptable is immutable the last key points to the next lookuptable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure can mod 3 and if there's remainder -> next key. can still add later w/o breaking change, as i don't expect those to fill up any time soon
const nullifyTable = | ||
await connection.getAddressLookupTable(nullifyTableAddress); | ||
if (!nullifyTable.value) { | ||
throw new Error('Nullify table not found'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Design doesn't account for cpi context accounts
My suggestion is:
- use one lookup table and assume that we store triplets
- we can do the same with tuples for the address trees in another pr (it's going to change to single accounts for BatchedAddress trees)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
const stateTreePubkeys = stateTreeLookupTable.value.state.addresses; | ||
const nullifyTablePubkeys = nullifyTable.value.state.addresses; | ||
|
||
// evens are state trees, odds are queues | ||
const activeStateTrees = stateTreePubkeys.filter( | ||
(_, index) => | ||
index % 2 === 0 && | ||
!nullifyTablePubkeys.includes(stateTreePubkeys[index]), | ||
); | ||
const activeQueues = stateTreePubkeys.filter( | ||
(_, index) => | ||
index % 2 !== 0 && | ||
!nullifyTablePubkeys.includes(stateTreePubkeys[index]), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const lookUpTablePubkey = stateTreeLookupTable.value.state.addresses;
let pubkeys = [];
for (let i = 0; i < pubkeys.length; i += 3) {
const tree = lookUpTablePubkey[i];
// Skip rolledover (full or almost full) Merkle trees
if !nullifyTablePubkeys.includes(tree) {
pubkeys.push({
tree,
queue: lookUpTablePubkey[i + 1],
cpi_context: lookUpTablePubkey[i + 2],
tree_type: TreeType.State,
});}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
good pointers. updated to use |
const transferAmount = 1e4; | ||
const numberOfTransfers = 15; | ||
|
||
it.only('account must have merkleTree2 and nullifierQueue2', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only leftover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, how do you plan to add support for different trees in the transfer instruction (I assume in a different pr)?
Another note, we will have fewer breaking changes when switching to batched trees if we use the active tree triple instead of passing Merkle tree pubkeys since with batched trees we need to pass the output queue instead of the Merkle tree pubkey which will be confusing. If we pass the triple and select by type in the function which pubkey we use in the instruction it will be easier to handle for devs.
d6da526
to
9b613d7
Compare
get-light-state-tree-info.ts wip wip added to stateless.js + test wip wip wip added: multi merkle support use math.random add changelog.md remove singleton, add easier createRpc invoke lint update to ActiveTreeBundle type fix lint update devnet and m-b lut keys remove only rm dead todo increas batch size in mint-to tests
667172f
to
0ed1a9d
Compare
problem
Added
added second tree+queue pair to CLI
added error in padStateTrees: at least an
inputAccount
oroutputstate
tree must be passed in from the outsideadded
defaultStateTreeLookupTables
const for mainnet and devnet (existing trees), andlocalTestActiveStateTreeInfo
for localnet. (limited to 2 trees and queues, no rpc call).internal ops helper actions
createStateTreeLookupTable
,extendStateTreeLookupTable
,nullifyLookupTable
.Works like this: by default, clients fetch our public state tree lookup. the lookup lists all active public state trees and associated queues. (since LUTs are append-only, we maintain a "nullifier" table and dedupe responses (see
getLightStateTreeInfo
). state trees are on evens, and their queues are stored at odds. We can deprecate this once Photon supports such a method.getLightStateTreeInfo
started
changelog.md
for js releasesadded utilities:
pickRandomStateTreeAndQueue
getLightStateTreeInfo
getTreeForQueue
getQueueForTree
(used in RPC client. todo: add to photon response)looks like this:
Breaking Changes
LightSystemProgram.createAccount
andCompressedTokenProgram.mintTo
) now require an explicit output state tree public key or input account, otherwise they will throw an error.Other Changes
createMint
allows passing of freezeAuthority in action (non-breaking)createMint
action now lets you pass tokenprogramId explicitly. (non-breaking with the boolean flag for t22)Deprecated
rpc.getValidityProof
. Now, another RPC round trip is made to fetch tree info. Instead, userpc.getValidityProofV0
and pass tree info explicitly to reduce latency.