Skip to content

p-token: Fix review comments #32

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

Merged
merged 7 commits into from
Mar 10, 2025
Merged

p-token: Fix review comments #32

merged 7 commits into from
Mar 10, 2025

Conversation

febo
Copy link
Contributor

@febo febo commented Mar 5, 2025

Address Jon's comments from #28.

Closes #28

@febo febo marked this pull request as ready for review March 6, 2025 02:26
@febo febo requested a review from joncinque March 6, 2025 02:27
@febo febo mentioned this pull request Mar 6, 2025
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks great! Mostly small things

@febo febo requested a review from joncinque March 6, 2025 19:33
@febo febo force-pushed the febo/p-token-scripts branch from 76bd300 to 64bd48f Compare March 7, 2025 22:49
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Just a couple of last bits, then this is good to go on my side

_data: PhantomData,
}),
2 if *bytes.get_unchecked(1) == 0 => Ok(&*(bytes.as_ptr() as *const SetAuthority)),
34 => Ok(&*(bytes.as_ptr() as *const SetAuthority)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to remove the if *bytes.get_unchecked(1) == 1 portion to this?

We probably want to keep that to be safe

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 issue is that it disproportionally increases CUs to have it. We could do the check on the accessor method:

pub fn new_authority(&self) -> Option<&Pubkey> {
    if self.new_authority.0 == 1 {
        Option::Some(&self.new_authority.1)
    } else {
        Option::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.

Or even return a Result?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine, but it has to be 1 or 0 or it errors, which means this would need to return Result<Option<&Pubkey>, ProgramError> instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think that is the best option. I will test it, but I can't imagine that it would add too much overhead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't do this and the current implementation has a different behaviour than the original one. We need to validate the instruction data before executing any code of the processor, otherwise we can get different results.

I fixed both SetAuthority and InitializeMint instruction data validation, adding the validation for the option byte.

34 if *bytes.get_unchecked(33) == 0 => {
Ok(&*(bytes.as_ptr() as *const InitializeMint))
}
66 => Ok(&*(bytes.as_ptr() as *const InitializeMint)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have if *bytes.get_unchecked(33) == 1 like the other one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here, we could have the check on the accessor instead.

@febo febo requested a review from joncinque March 8, 2025 02:43
@febo febo force-pushed the febo/p-token-scripts branch from 64bd48f to 7981197 Compare March 8, 2025 03:00
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks great!

@febo febo force-pushed the febo/p-token-scripts branch from 7981197 to bc765ed Compare March 10, 2025 14:12
Base automatically changed from febo/p-token-scripts to main March 10, 2025 14:24
@febo febo force-pushed the febo/fix-jon-comments branch from e058e93 to c12b54f Compare March 10, 2025 14:40
@febo febo merged commit 3daf448 into main Mar 10, 2025
11 checks passed
@febo febo deleted the febo/fix-jon-comments branch March 10, 2025 14:54
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