-
Notifications
You must be signed in to change notification settings - Fork 308
[message-buffer 10/X] - Message Buffer Admin IXs & variable length #779
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
| .realloc(target_size, false) | ||
| .map_err(|_| MessageBufferError::ReallocFailed)?; | ||
| } else { | ||
| // TODO: |
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.
@jayantk what are your thoughts on this?
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 think it's fine to shrink and not worry about the lamports. just leave a comment that the account will retain more rent than needed
| ) | ||
| } | ||
|
|
||
| // TODO: should delete_buffer have same CPI restrictions |
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.
@jayantk do we want to be able to call delete_buffer outside of a pyth delete_XXX ix?
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.
yeah let the admin delete whatever.
can you add doc comments to all of these instructions?
|
|
||
| /// Initializes the buffer account with the target_size | ||
| /// | ||
| /// TODO: should target_size be a parameter or should |
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.
@jayantk what are your thoughts on this?
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.
target_size parameter is good. I think we have to manage the sizes anyway
jayantk
left a comment
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.
Generally looks good to me with a bunch of minor comments. I think in the interest of time, please fix the API (specifically the unnecessary system_program args) so we can integrate, merge, and then address feedback in a subsequent PR.
| *accumulator_input = MessageBuffer::new(bump); | ||
| } | ||
| loader.exit(&crate::ID)?; | ||
| } |
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.
maybe we can log a warning or something if the account already exists?
Summary
Add
create/resize/delete_bufferixs, allow forMessageBufferto be variable lengthChanges
fundwhitelist.authoritytowhitelist.adminand check that admin is signer & payer for all admin ixs.MessageBufferstruct now only defines the header fields and allows for variable length for the actual messages (seeMessageBuffer::put_all_in_buffer)