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

ported memo to anchor #3024

Closed
wants to merge 3 commits into from

Conversation

Saviour1001
Copy link
Contributor

  • Wrote the Anchor version of the Memo program
  • Wrote the tests for the same as per this
  • Works as expected, deployed on devnet

Deployed on Devnet: 4ARKJpztrkSJXJQ8ii9pr6NjJdwKrC6M3jA8LG21tvjG

@joncinque

@mergify mergify bot added the community Community contribution label Mar 23, 2022
@joncinque
Copy link
Contributor

Sorry, I was getting a review ready off of #3020 -- is there any difference here?

@mvines
Copy link
Member

mvines commented Mar 23, 2022

This work is nice however I don't see a reason to land it in SPL.

@Saviour1001
Copy link
Contributor Author

Sorry, I was getting a review ready off of #3020 -- is there any difference here?

No difference, these are the changes, had some problems with the commit history of #3020. This is the only PR

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 really great for the most part! A few small points to address on your side, and on our side we'll figure out the correct repo for this to land.

Another thing to consider is to run cargo fmt to be sure the code is automatically formatted, and cargo clippy -Zunstable-options --workspace --all-targets -- --deny=warnings

memo/anchor_program/memo/src/lib.rs Outdated Show resolved Hide resolved
memo/anchor_program/memo/src/lib.rs Outdated Show resolved Hide resolved
memo/anchor_program/memo/src/lib.rs Outdated Show resolved Hide resolved
pub mod memo {
use super::*;
use std::str;
pub fn build_memo(ctx: Context<BuildMemo>,input:Vec<u8>) -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the name build_memo is a bit misleading, since you're actually printing the memo in your program, and not building it. Maybe log_memo would be a better name?

Also, for input, since it could be a large buffer, it's better to accept a &[u8] rather than a Vec<u8> to avoid copying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Changed the function name to log_memo
  • Anchor deserialize function won't allow the input to be &[u8], tried a lot of different ways to make it happen but well Anchor complains a lot in this particular scenario.
    Error:
    the function or associated item `deserialize` exists for struct `instruction::LogMemo`, but its trait bounds were not satisfied

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this might be the place for zero-copy deserialization! Here's an example for you to follow: https://github.com/project-serum/anchor/blob/master/tests/zero-copy/programs/zero-copy/src/lib.rs

memo/anchor_program/tests/memo.js Outdated Show resolved Hide resolved
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.

One more nit, otherwise just need to figure out whether it's worth going zero-copy

memo/anchor_program/memo/src/lib.rs Outdated Show resolved Hide resolved
@jnappy
Copy link

jnappy commented Jun 14, 2022

@Saviour1001 - is this worth a bump? cc @joncinque

@mvines
Copy link
Member

mvines commented Jun 14, 2022

Just to be clear, I don't think we should land this PR. It can live as a separate repo as an example of how to use Anchor, but I have no opinion on where beyond not in the SPL repo

@Saviour1001
Copy link
Contributor Author

Let me know where to shift this PR and I will do it.

@github-actions github-actions bot added the stale [bot only] Added to stale content; will be closed soon label Aug 10, 2023
@github-actions github-actions bot closed this Aug 18, 2023
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