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

Solana plugin codec implementation #15816

Open
wants to merge 51 commits into
base: solana-offchain-plugin
Choose a base branch
from

Conversation

huangzhen1997
Copy link
Contributor

@huangzhen1997 huangzhen1997 commented Dec 30, 2024

Jira reference:
https://smartcontract-it.atlassian.net/browse/NONEVM-935

The codec is implemented using chainlink-ccip gobinding tool.

The onchain sdk will provide a calculation for the token pool accounts, right now it's a fixed account length based on https://github.com/smartcontractkit/chainlink-ccip/blob/main/chains/solana/contracts/tests/ccip/ccip_router_test.go#L4839

Copy link
Contributor

github-actions bot commented Dec 30, 2024

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@huangzhen1997 huangzhen1997 marked this pull request as ready for review January 3, 2025 21:56
@huangzhen1997 huangzhen1997 requested review from a team as code owners January 3, 2025 21:56
@huangzhen1997 huangzhen1997 changed the base branch from develop to solana-offchain-plugin January 9, 2025 19:44
core/capabilities/ccip/ccipsolana/commitcodec.go Outdated Show resolved Hide resolved
DestTokenAddress: DestTokenAddress,
ExtraData: tokenAmount.ExtraData,
Amount: bigIntToBytes32(tokenAmount.Amount),
DestGasAmount: bytesToUint32(tokenAmount.DestExecData),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not fully sure what format this field will be encoded to []byte as.
We should review this specific field with a CCIP team member.
I suspect if source chain is EVM, this may be encoded using abi.


var extraArgs ccip_router.SolanaExtraArgs
decoder := agbinary.NewBorshDecoder(msg.ExtraArgs)
err := extraArgs.UnmarshalWithDecoder(decoder)
Copy link
Contributor

Choose a reason for hiding this comment

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

I started a thread around SolanaExtraArgs encoding scheme.
As of today, I think if EVM is the source chain, this would be encoded as abi.
We need to discuss in that thread what this should be like.

tokenAmounts := make([]cciptypes.RampTokenAmount, 0, len(executeReport.Message.TokenAmounts))
for _, tokenAmount := range executeReport.Message.TokenAmounts {
destData := make([]byte, 4)
binary.BigEndian.PutUint32(destData, tokenAmount.DestGasAmount)
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I'm a little lost on what this encoding should be like.
Need someone from CCIP to explain to us.

core/capabilities/ccip/ccipsolana/executecodec.go Outdated Show resolved Hide resolved
core/capabilities/ccip/ccipsolana/executecodec.go Outdated Show resolved Hide resolved
// CommitPluginCodecV1 is a codec for encoding and decoding commit plugin reports.
// Compatible with:
// - "OffRamp 1.6.0-dev"
type CommitPluginCodecV1 struct{}
Copy link
Contributor

@ilija42 ilija42 Jan 14, 2025

Choose a reason for hiding this comment

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

why can't we use the Solana Codec for this? Just define the IDL for these types, I haven't looked closely into the types being encodec, so I may be missing something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could try Solana codec, @archseer suggested to use the generated gobinding for this earlier, and I also think it makes sense as the EVM codec right now is using similar approach with the generated type.

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