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

added method to form message using chunks #165

Closed
wants to merge 6 commits into from
Closed

Conversation

dhruvja
Copy link
Collaborator

@dhruvja dhruvja commented Dec 23, 2023

Since instructions to solana have a limit of 1232 bytes, we cannot send messages which exceed the limit. Since the update headers and other messages can easily exceed the limit, we have to split them into chunks and add it to an account so that it can be accessed later in the deliver method.

Once the data in the account in executed by the deliver, the account data ( which stored the msg chunks) would be cleared.

@dhruvja dhruvja requested a review from mina86 December 23, 2023 17:39
Comment on lines +153 to +159
let cloned_msg_chunks: &mut MsgChunks = &mut msg_chunks.clone();
let mut store = storage::from_ctx!(ctx, with accounts);
let mut router = store.clone();

let message = ibc::MsgEnvelope::try_from(ibc::Any::from(
cloned_msg_chunks.clone(),
))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we clone msg_chunks twice? Why is cloned_msg_chunks an exclusive reference?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

its a reference because i needed msg_chunks as MsgChunks so that i could convert it. But i think we can ignore it as we are doing the deserializing stuff in the form_msg_chunks method itself. So we would just be fetching the MsgEnvelope from the account and hence no conversions would be required in the deliver_chunks method.

Comment on lines +596 to +597
#[account(init_if_needed, payer = sender, seeds = [MSG_CHUNKS], bump, space = 10240)]
pub msg_chunks: Account<'info, storage::MsgChunks>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Two things.

First of all, do we need this to be a PDA? I was actually wondering if it wouldn’t be better if this was just an account owned by the sender and then they can manage it in any way they want. All we need here is a writeable account.

Second of all, if this is a PDA it must include sender in the seeds. Without that, users will overwrite each others calls.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes i was thinking about it too. I was also looking if we could create an account with data outside of smart contract. Couldnt find anything as now. But we dont need PDA, we could use the sender's account too. Once the deliver_chunks is called, we can also close the account so that we can return the rent.

Comment on lines +382 to +391
/// Struct containing message chunks
///
/// The struct consists of chunks of message data having the first 4 bytes
/// indicating the total size of the message
#[account]
#[derive(Debug)]
pub struct MsgChunks {
pub type_url: String,
pub value: Vec<u8>,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This really should be zero copy or unchecked account IMO. Furthermore, rather than giving it a structure with type url and value, it’s better to treat it as a length-prefixed sequence of bytes. Those can than be decoded as proto to get Any message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the value is length prefixed but we would still need type_url right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No. The value would be an Any proto message so it would include type url.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh i see, that makes sense.

@dhruvja
Copy link
Collaborator Author

dhruvja commented Jan 30, 2024

Closing this since we would be using #171 .

@dhruvja dhruvja closed this Jan 30, 2024
@mina86 mina86 deleted the deliver-chunks branch April 25, 2024 12:02
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