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

WIP: Init With Root #6441

Closed
wants to merge 4 commits into from
Closed

WIP: Init With Root #6441

wants to merge 4 commits into from

Conversation

austbot
Copy link
Contributor

@austbot austbot commented Mar 15, 2024

This pr serves as another attempt to get init with root in. This PR is wip as tests are still being written. But would like to have buy in on the approach.

The approach is as follows, you can init a tree with a root and the proof of the first filled leaf, leaf at index 0. For huuuuuuuuuge trees there is a buffer mechanism to allow you to fill the canopy with more proofs than would fit in a transaction and then you can init.

Im hoping minimize code changes and work within the existing framework, ensure compatibility with indexers.

@mergify mergify bot added the community Community contribution label Mar 15, 2024
@joncinque joncinque requested a review from ngundotra March 19, 2024 12:06
Copy link
Contributor

@ngundotra ngundotra left a comment

Choose a reason for hiding this comment

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

Can you uncommit the lint & do in a separate PR? Makes it difficult to see your changes.

Left a bunch of comments

@mergify mergify bot dismissed ngundotra’s stale review March 19, 2024 16:31

Pull request has been modified.

Copy link
Contributor

@danenbm danenbm left a comment

Choose a reason for hiding this comment

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

Looking good! Left a few small comments. Note CI is failing now.

Comment on lines +678 to +685
"code": 6009,
"name": "ProofIndexOutOfBounds",
"msg": "Proof index of is out of bounds"
},
{
"code": 6010,
"name": "InvalidProofBuffer",
"msg": "Invalid proof buffer"
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like IDL and SDK need to be regenerated after latest changes

Comment on lines 44 to +52
export const changeLogEventBeet = beet.dataEnum<ChangeLogEventRecord>([
[
'V1',
new beet.FixableBeetArgsStruct<ChangeLogEventRecord['V1']>(
[['fields', beet.tuple([changeLogEventV1Beet])]],
'ChangeLogEventRecord["V1"]',
),
],
]) as beet.FixableBeet<ChangeLogEvent>;
[
'V1',
new beet.FixableBeetArgsStruct<ChangeLogEventRecord['V1']>(
[['fields', beet.tuple([changeLogEventV1Beet])]],
'ChangeLogEventRecord["V1"]'
),
],
]) as beet.FixableBeet<ChangeLogEvent, ChangeLogEvent>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Has the Solita autogenerated code been run through a different linter? It seems so because afaik Solita generates with commas and semicolons. I would recommend regenerating the Solita TS and not linting it, in order to keep it consistent with other generated Solita code and minimize changed files for the next PR. (I do agree the 2-space indent belongs, and I think Solita does that by default).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they asked me to remove the lint

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah got it, I think that @ngundotra just didn't want to see all the lint changes as part of this PR, but @StanChe and I were looking into how to actually get the format that is checked in.

It looks like by default the prettierrc.yaml file is not used, so you have to do something like this:

pnpm solita
pnpm lint:fix
pnpm prettier --config prettierrc.yaml  --write idl/spl_account_compression.json

I still see some slight differences in the output and had some error messages in the output, but it did work.
@StanChe might have gotten cleaner results but wasn't sure of his local setup.

Having said all this, I think the bigger issue is the missing canopy issue described below in @StanChe 's comment. This could be a blocker for the PR and I think @StanChe is looking into a solution to initialize the canopy on a tree that uses one.

crate::id(),
AccountCompressionError::IncorrectAccountOwner
);
let mut merkle_tree_bytes: std::cell::RefMut<'_, &mut [u8]> =
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the type added for merkle_tree_bytes? Couldn't it just be let mut merkle_tree_bytes = ctx.accounts.merkle_tree..."

Comment on lines +74 to +87
/// Context for initializing a new SPL ConcurrentMerkleTree
#[derive(Accounts)]
pub struct InitializeWithRoot<'info> {
#[account(zero)]
/// CHECK: This account will be zeroed out, and the size will be validated
pub merkle_tree: UncheckedAccount<'info>,

/// Authority that controls write-access to the tree
/// Typically a program, e.g., the Bubblegum contract validates that leaves are valid NFTs.
pub authority: Signer<'info>,

/// Program used to emit changelogs as cpi instruction data.
pub noop: Program<'info, Noop>,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just use Initialize as its a duplicate struct?

max_depth: u32,
max_buffer_size: u32,
root: [u8; 32],
leaf: [u8; 32], // the first leaf
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: comment or param name should be rightmost_leaf imo as "first leaf" would only be rightmost if it was empty?

@danenbm
Copy link
Contributor

danenbm commented May 14, 2024

Can you uncommit the lint & do in a separate PR? Makes it difficult to see your changes.

Left a bunch of comments

+1 to undoing the lint. In my review I suggested linting in a way that matches Solita output as well (I think this linting differs slightly from Solita generated output, but I could be mistaken)

@StanChe
Copy link
Contributor

StanChe commented May 15, 2024

Unfortunately this change doesn't take into account canopy. If the tree account is created with space for canopy, and the init with root method is called, the proofs provided to modify the assets will be shorter and will only represent the proof under the canopy. Only the last leaf will have the canopy cached properly, while the rest of the canopy will be considered empty. Thus the proofs under any other then the last canopy node will be invalid. I could take over from here and provide an option to init the canopy before the root is provided and also verify the canopy is properly initialized (has all the nodes to the left of the last leaf) before allowing the initialization.

@github-actions github-actions bot added the stale [bot only] Added to stale content; will be closed soon label Jun 3, 2024
@github-actions github-actions bot closed this Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution stale [bot only] Added to stale content; will be closed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants