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

fix(svm): N-03 remove duplicate logic #845

Merged
merged 3 commits into from
Jan 20, 2025

Conversation

Reinis-FRP
Copy link
Contributor

@Reinis-FRP Reinis-FRP commented Jan 7, 2025

OZ identified following issue:

The claim_relayer_refund and claim_relayer_refund_for functions share almost identical logic, with the primary distinction being the refund address. This duplication introduces maintainability challenges, as future updates may need to be applied to both functions simultaneously, increasing the risk of inconsistencies.

To streamline the code and reduce redundancy, consider implementing the following refactor:

Abstract the common logic from both functions into a helper function that handles the transfer and event emission. This helper could accept parameters such as the target token account and any additional context-specific data.
Alternatively, consolidate the instructions into a single function that takes an optional refund_address parameter, enabling flexible use cases while maintaining a single code path.
Implementing either of these approaches would simplify future maintenance and reduce the surface area for potential bugs.

This PR addresses the issue by consolidating the instructions into a single function that takes additional refund_address account. When it is different from the signer, the receiving token_account must match ATA for this refund_address. Otherwise it can be any custom token account for the given mint and token program as the claim account would be derived from the signer.

Signed-off-by: Reinis Martinsons <[email protected]>
Copy link

linear bot commented Jan 7, 2025

@@ -13,3 +17,17 @@ pub fn is_local_or_remote_owner(signer: &Signer, state: &Account<State>) -> bool
pub fn is_relay_hash_valid(relay_hash: &[u8; 32], relay_data: &V3RelayData, state: &Account<State>) -> bool {
relay_hash == &get_v3_relay_hash(relay_data, state.chain_id)
}

// Implements the same underlying logic as in Anchor's associated_token constraint macro, except for token_program_check
// as that would duplicate Anchor's token constraint macro that the caller already uses.
Copy link
Contributor

Choose a reason for hiding this comment

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

as that would duplicate Anchor's token constraint macro that the caller already uses.

You mean that the token constraint macro is already applied here?

token::token_program = token_program,

md0x
md0x previously approved these changes Jan 7, 2025
Copy link
Contributor

@md0x md0x left a comment

Choose a reason for hiding this comment

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

Nice! Beautiful refactoring.

Signed-off-by: Reinis Martinsons <[email protected]>
@Reinis-FRP Reinis-FRP merged commit de821db into master Jan 20, 2025
9 checks passed
@Reinis-FRP Reinis-FRP deleted the reinis/acx-3592-n-03-duplicate-logic branch January 20, 2025 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants