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

Feat/split receiver #448

Merged
merged 21 commits into from
Jan 21, 2025
Merged

Feat/split receiver #448

merged 21 commits into from
Jan 21, 2025

Conversation

agusaldasoro
Copy link
Contributor

@agusaldasoro agusaldasoro commented Jan 15, 2025

This pull request includes significant changes to the ccip-router program in the Solana blockchain, focusing on refactoring the receiver handling by splitting it into token_receiver and logic_receiver. The changes affect multiple files, including core logic, tests, and documentation.

These changes aim to improve the clarity and functionality of the ccip-router program by clearly distinguishing between the logic execution receiver and the token transfer receiver.

// 1. There is at least one account used for messaging (the first subset of accounts). This is because the first account is the program id to do the CPI
// 2. AND the logic_receiver has a value different than zeros
fn should_execute_messaging(logic_receiver: &Pubkey, remaining_accounts_empty: bool) -> bool {
!remaining_accounts_empty && *logic_receiver != Pubkey::default()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we check here for the compute Units as well?

@agusaldasoro agusaldasoro requested a review from aalu1418 January 17, 2025 16:59
message.ExtraArgs.Accounts = []solana.PublicKey{
config.CcipReceiverProgram,
config.ReceiverTargetAccountPDA, // writable (index = 1)
config.ReceiverExternalExecutionConfigPDA, // writable (index = 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's weird why the tests did pass before this change 🤔

@agusaldasoro agusaldasoro marked this pull request as ready for review January 17, 2025 17:12
@agusaldasoro agusaldasoro requested a review from a team as a code owner January 17, 2025 17:12
// In EVM receiver means the address that all the listed tokens will transfer to and the address of the message execution.
// In Solana the receiver is split into two:
// Logic Receiver is the Program ID of the user's program that will execute the message
pub logic_receiver: Pubkey,
Copy link
Contributor

Choose a reason for hiding this comment

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

Wdyt about naming this message_receiver instead of logic_receiver? It feels closer to CCIP terminology in general, e.g. ccip use cases include "pure messaging" (thus message_receiver) vs "token transfer" vs "programmable token transfer" (which is the sum of the previous two)

Alternatively, using more of the Solana terminology, we could name it cpi_receiver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is actually no term for CCIP to distinguish the receiver between logic and tokens as for EVM they are both the same.

Naming them logic_receiver and token_receiver makes it impossible to have ambiguity between the two terms. Someone could argue that any CPI call is a message receiver or cpi receiver, those are more generics names.

Copy link
Contributor

@PabloMansanet PabloMansanet left a comment

Choose a reason for hiding this comment

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

Looks sensible to me, but I'm still unsure about duplications between the fields in SVM2AnyMessage and the entries of remaining_accounts . Seeing as at least the logic_receiver (and potentially also token_receiver? Not sure) needs to be in remaining_accounts, maybe we should not have any Pubkey in Any2SVMRampMessage and nip the duplication in the bud. WDYT?

receiver: Pubkey,
source_accounts: &[Pubkey],
logic_receiver: Pubkey,
extra_args_accounts: &[Pubkey],
Copy link
Contributor

Choose a reason for hiding this comment

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

I got rid of all of these here, just as a heads up to make sure what we've done works well together (I don't see why not: I don't mind rebasing on you if this gets merged first)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is aming for clarity on the users and the protocol itself, it doesn't add any functionality. We need to establish an abstraction that is super clear for the users/offchain code what to send, and the receiver (token or logic) is the most sensitive part of the protocol, we can't have any missunderstanding or wrong configuration. We could only receive accounts and do all the validations, but this way we are sure that all the nodes have signed those addresses as logic and token receivers.

Copy link

Metric feat/split-receiver main
Coverage 76.7% 76.6%

@aalu1418 aalu1418 merged commit e42d6d0 into main Jan 21, 2025
17 checks passed
@aalu1418 aalu1418 deleted the feat/split-receiver branch January 21, 2025 17:27
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.

4 participants