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: Update docs on Solana Ccm fallback mechanism #413

Merged
merged 4 commits into from
Oct 29, 2024

Conversation

syan095
Copy link
Contributor

@syan095 syan095 commented Oct 24, 2024

Closes PRO-1719

Updated documentation about Solana Ccm's fallback mechanism.
Corresponding code change is here: chainflip-io/chainflip-backend#5316

Copy link

vercel bot commented Oct 24, 2024

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

Name Status Preview Comments Updated (UTC)
chainflip-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 29, 2024 6:40am

- The receiver program on the destination chain must implement the [described interface](#receive-call-and-asset-on-the-receiver-program).
- If the amount of gas provided is not enough to cover the costs of the receiver's logic on the destination chain, the transaction will revert on-chain. An example of a compute unit estimation is provided [below](#compute-budget-estimation).
- In the event of a transaction reverting due to insufficient gas the nonce will be consumed making the transaction invalid.


<Callout type="warning">

While Chainflip will do it's best to succesfully execute any swap with cross-chain messaging, CCM transactions that revert on Solana will currently result in funds not being egressed. Ensure the transaction doesn't revert due to lack of compute units or due to the receiver's logic.
While Chainflip will do its best to successfully execute any swap with cross-chain messaging, CCM transactions that revert on Solana will currently result in funds not being egressed. Ensure the transaction doesn't revert due to a lack of "compute units" or the receiver's logic.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that now that there is the fallback mechanism we don't need to flag this as a warning, we could put it as part of the Solana Considerations.

Comment on lines 47 to 56
pub struct CcmAddress {
pub pubkey: Pubkey,
pub is_writable: bool,
}

pub struct CcmAccounts {
pub cf_receiver: CcmAddress,
pub remaining_accounts: Vec<CcmAddress>,
pub fallback_address: 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 think it would be more accessible and precise if, in the docs, we used primitive types instead of our own internal type definitions, so something like:

cf_receiver: { pubkey: [u8; 32], is_writable: u8 }
remaining_accounts: Array<{ pubkey: [u8; 32], is_writable: u8 }>
fallback_address: [u8; 32]

We could even provide the scale metadata?

Otherwise it's easy to assume that eg. the address is base58-encoded.

We could also link to some SCALE docs and common libraries, and provide some example code in eg. TS or Python, as we do elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's totally true, especially relevant for the address encoding you mentioned.
Regarding the scale encoding library that makes sense but I would maybe do it also as part of extending the docs for Vault swaps as they will also need the same encoding, especially since I don't think anyone is on a rush to implement that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah not saying all of the above has be part of this PR, but we should at least use non-custom types in the example.

Having said that, most integrators don't need to know this: they should be able to just request the encoding via an rpc, and then get back some encoded data that they just need to slot into the tx somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have such a RPC at the moment?

Copy link
Contributor Author

@syan095 syan095 Oct 29, 2024

Choose a reason for hiding this comment

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

I had the raw Rust code in there just for reference, in case someone wants to get "technical".

I've added the "encoded" type - I'm unsure if what I had in there makes sense.
Should we have more detailed instructions on how to get everything encoded? (either JS code with scale or the RPC call that do the encoding for the users)

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have the rpc yet so it's good to document it. I think it looks good now, we can do the scale encoding when I expand the contract swaps.

@albert-llimos
Copy link
Contributor

As I understand this feature is in 1.7. Should we merge it anyway so any party that is looking into this has already the "final implementation" even though this won't work in main yet? @dandanlen

@syan095 syan095 merged commit c45f325 into main Oct 29, 2024
2 checks passed
@syan095 syan095 deleted the feat/solana-ccm-fallback branch October 29, 2024 20:45
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