Skip to content

feat: optional memo#458

Merged
guibescos merged 16 commits intomainfrom
fea/memo
Mar 28, 2025
Merged

feat: optional memo#458
guibescos merged 16 commits intomainfrom
fea/memo

Conversation

@guibescos
Copy link
Copy Markdown
Contributor

No description provided.

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 26, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
swap-staging ⬜️ Ignored (Inspect) Visit Preview Mar 27, 2025 6:36pm

@guibescos guibescos marked this pull request as draft March 26, 2025 15:57
@guibescos guibescos marked this pull request as ready for review March 26, 2025 20:14
}
}

const MEMO_MAX_LENGTH: usize = 100;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

decide a reasonable value here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this to prevent DoSooor quote requestoors from making a bunch of illegitimate quote requests with too-long-to-be-included-in-a-transaction memos?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if they pass a very long memo, they will just get 404 because the searcher transactions will be too big.
it's better to fail explicitly here so the user understands the reason for not getting any quotes is the length of the memo.

Comment thread auction-server/src/auction/service/verification.rs Outdated
Comment thread auction-server/src/auction/service/verification.rs Outdated
Comment thread auction-server/src/auction/service/verification.rs Outdated
Comment thread auction-server/src/auction/service/verification.rs Outdated
Comment thread auction-server/src/auction/service/verification.rs Outdated
Comment thread sdk/js/src/svm.ts
searcherToken: PublicKey;
tokenProgramSearcher: PublicKey;
tokenInitializationConfigs: TokenAccountInitializationConfigs;
memo?: string;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe we should make this return type into a struct?

Copy link
Copy Markdown
Contributor Author

@guibescos guibescos Mar 27, 2025

Choose a reason for hiding this comment

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

It's okay imo. it's already a string on the server side.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i mean the whole return type, there's a lot of fields in there

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This type gets deconstructed right away everywhere so i'll just leave it as it is.

Comment thread sdk/python/express_relay/client.py
Comment thread sdk/js/src/svm.ts
const { userToken, searcherToken, user, tokenInitializationConfigs } =
extractSwapInfo(swapOpportunity);

if (swapOpportunity.memo) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe we want to make this backward compatible for limo/submit bid as well?

Copy link
Copy Markdown
Contributor Author

@guibescos guibescos Mar 27, 2025

Choose a reason for hiding this comment

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

do you mean adding this to limit orders? let's decide if we need to prioritize that, we should do it in a different pull request anyway

Ok(())
}
(Some(_), 0) => Ok(()), // todo: this is for backward compatibility, we should remove this line once searchers have updated their sdk
(_, _) => Err(RestError::InvalidInstruction(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

one other thing to think about is what if searchers use memo instructions of their own in the program? there's probably not a reason to fail the tx if it has the required memo program ix and then others on top of that

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let's wait until a searcher requests this before doing it.

}
}

const MEMO_MAX_LENGTH: usize = 100;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this to prevent DoSooor quote requestoors from making a bunch of illegitimate quote requests with too-long-to-be-included-in-a-transaction memos?

@guibescos guibescos merged commit 12a33dd into main Mar 28, 2025
3 checks passed
@guibescos guibescos deleted the fea/memo branch March 28, 2025 16:56
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