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

chore: review-cleanup #929

Merged
merged 3 commits into from
Jul 2, 2024
Merged

chore: review-cleanup #929

merged 3 commits into from
Jul 2, 2024

Conversation

ananas-block
Copy link
Contributor

@ananas-block ananas-block commented Jul 1, 2024

Changes:

  • removed unused errors
  • nullify leaves rename indices -> leaf_indices
  • register program replace address constraint with constraint with same functionality because anchor 0.30. idl generation doesn't like address constraints
  • update_address_merkle_tree - add comments, move address indexed element down, replace sequence_number with merkle_tree.sequence_number after it has been incremented
  • append_state refactor index_merkle_tree_account to start at zero and be incremented with every iteration
  • invoke/processor.rs - rename roots to input_compressed_account_roots
  • invoke/processor.rs - remove deduplication of sequence numbers, its not needed anymore since we enforce ordered output Merkle trees
  • rename fetch_roots -> fetch_input_compressed_account_roots
  • hash_input_compressed_accounts -> enforce input ordering because we allow Merkle trees to be inserted into hashed_pubkeys without checking whether these exist already
  • merkle tree append performance has regressed by 1 from 141 to 140 for unknown reasons (no changes in append_leaves in account compression program)

@ananas-block ananas-block requested a review from vadorovsky as a code owner July 1, 2024 23:30
@ananas-block ananas-block force-pushed the jorrit/review-cleanup branch from 984de4b to ee6f87a Compare July 1, 2024 23:47
@ananas-block ananas-block force-pushed the jorrit/review-cleanup branch 2 times, most recently from 6c14282 to a28e3fe Compare July 2, 2024 04:15
@ananas-block ananas-block force-pushed the jorrit/review-cleanup branch from a28e3fe to c2fa8f9 Compare July 2, 2024 04:16
},
{
name: 'solPoolPda';
isMut: true;
isSigner: false;
isOptional: true;
docs: [
'Sol pool pda is used to store compressed sol.',
Copy link
Contributor

@SwenSchaeferjohann SwenSchaeferjohann Jul 2, 2024

Choose a reason for hiding this comment

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

doesn't actually store compressed sol but native sol, so may be confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Joa both not wrong, the native sol that has been compressed.
Makes sense to be explicit let me fix it in https://github.com/Lightprotocol/light-protocol/actions/runs/9754900751/job/26922536668?pr=931 since the tests are already green here.

},
{
name: 'accountCompressionProgram',
isMut: false,
isSigner: false,
docs: ['Merkle trees.'],
Copy link
Contributor

Choose a reason for hiding this comment

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

unclear docs

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 is an error in the idl the full comment reads:

/// CHECK: Account compression program is used to update state and address
    /// Merkle trees.
    ``` 

programs/system/src/invoke/processor.rs Show resolved Hide resolved
Comment on lines 32 to 37
if inputs.is_compress {
compress_lamports(inputs, ctx)
} else if inputs.compress_or_decompress_lamports.is_some() {
decompress_lamports(inputs, ctx)
} else {
Ok(())
decompress_lamports(inputs, ctx)
}
}
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 now by default always going to execute decompress_lamports(),no?
is_compress is not optional, so the default usage is is_compress = false, compress_or_decompress_lamports = None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function yes but we only execute the function when compress_or_decompress_lamports is some.
Hence, this function will never be executed if we don't want to compress or decompress.

@@ -61,9 +59,7 @@ pub fn decompress_lamports<
None => return err!(SystemProgramError::DeCompressLamportsUndefinedForDecompressSol),
};

transfer_lamports(&sol_pool_pda, &recipient, lamports)?;

Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make any difference to not emit Ok(()) here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's cleaner not to

Copy link
Contributor

@SwenSchaeferjohann SwenSchaeferjohann left a comment

Choose a reason for hiding this comment

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

approving, required changes will be addressed in #931

@SwenSchaeferjohann SwenSchaeferjohann merged commit fb96412 into main Jul 2, 2024
13 checks passed
@ananas-block ananas-block deleted the jorrit/review-cleanup branch July 2, 2024 05:22
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.

2 participants