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

[WIP] Family Specific Transmitters #16704

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

0xsuryansh
Copy link
Member

@0xsuryansh 0xsuryansh commented Mar 10, 2025

Created a ContractTransmitterFactory interface with two functions

  1. NewCommitTransmitter
  2. NewExecTransmitter

This interface has two implementation EVMContractTransmitterFactory and SVMContractTransmitterFactory with chain family specific ToCalldataFunc

Copy link
Contributor

github-actions bot commented Mar 10, 2025

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@0xsuryansh 0xsuryansh changed the title Family Specific Transmitters [WIP] Family Specific Transmitters Mar 10, 2025
… into CCIP-5276_family_specific_transmitters

# Conflicts:
#	core/capabilities/ccip/ocrimpls/contract_transmitter.go
#	core/capabilities/ccip/oraclecreator/plugin.go
@0xsuryansh 0xsuryansh requested review from a team as code owners March 11, 2025 09:24
@0xsuryansh 0xsuryansh requested a review from ilija42 March 11, 2025 09:24
@0xsuryansh 0xsuryansh changed the title [WIP] Family Specific Transmitters Family Specific Transmitters Mar 11, 2025
Comment on lines +10 to +11
// ContractTransmitterFactory creates commit/execute transmitters for a specific chain family.
type ContractTransmitterFactory interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like it would make more sense to define this in ocrimpls

Copy link
Member Author

Choose a reason for hiding this comment

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

My thought is that this is a factory interface for different transmitters , the implementation of which exist in the impl folder ?

Comment on lines +11 to +14
type ExtraDataDecoded struct {
ExtraArgsDecoded map[string]any
DestExecDataDecoded []map[string]any
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have doc comments on public methods, members, and types that are introduced?

offrampAddress string,
defaultMethod, priceOnlyMethod string,
) ocr3types.ContractTransmitter[[]byte] {
return &ccipTransmitter{
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we using the same exact object for all these methods? The point of the separate transmitter per family is to literally have a different implementation. I don't think this PR really improves much upon the baseline.

Copy link
Member Author

Choose a reason for hiding this comment

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

The design intentionally uses the same underlying transmitter type, but the key is that its behavior is driven by the injected toCalldataFn. Each factory provides its own implementation of this function, which tailors the calldata encoding and ultimately the transaction submission logic to the specific chain (EVM or Solana).

While the outer object is the same for common functionality the chain-specific logic lives in the callback.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this approach at all and I was the one that (unfortunately) introduced it.

@0xsuryansh 0xsuryansh changed the title Family Specific Transmitters [WIP] Family Specific Transmitters Mar 11, 2025
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.

3 participants