feat: use Solana versioned transaction to build api calls #5633
+735
−635
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Moved ALT related stuff to its own file.
albert-llimos
force-pushed
the
feat/solana-versioned-transaction
branch
from
ec95f58 to
56a6108
Compare
Removed all uses of Legacy message and Transaction.
syan095
requested review from
albert-llimos,
dandanlen and
martin-chainflip
as code owners
syan095
changed the title
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
I think it looks great overall, pending on how exactly we implement the rest of the logic. Few comments:
- I see you have removed some of the legacy transaction logic. I guess that's fine and we don't plan on using it anywhere.
- As I mentioned to you, in feat: add image with ALT Manager, additional nonces and ALT witnessing. #5638 I have simplified a bit the Chainflip ALT logic to include it in the Solana Environment.
- That other PR is now ready for review. We can merge this onto the base branch or wait to merge the other one on top of this and then merge it into the main branch.
- Do we need any migration? It feels like we might need one since the transacion and messages have changed 🤔 (I haven't had the time to check)
I just added some minor comments.
Comment on lines
+172
to
+183
| fn decode_unchecked( | ||
| ccm: &CcmChannelMetadata, | ||
| chain: ForeignChain, | ||
| ) -> Result<DecodedCcmAdditionalData, CcmValidityError> { | ||
| if chain == ForeignChain::Solana { | ||
| VersionedSolanaCcmAdditionalData::decode(&mut &ccm.ccm_additional_data.clone()[..]) | ||
| .map(DecodedCcmAdditionalData::Solana) | ||
| .map_err(|_| CcmValidityError::CannotDecodeCcmAdditionalData) | ||
| } else { | ||
| Ok(DecodedCcmAdditionalData::NotRequired) | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't see this being called anywhere except tests. Am I missing something?
| @@ -129,6 +129,7 @@ where | |||
| _gas_budget: GasAmount, | |||
| _message: Vec<u8>, | |||
| _ccm_additional_data: Vec<u8>, | |||
| _swap_request_id: SwapRequestId, | |||
There was a problem hiding this comment.
Just fyi, @ramizhasan111 is looking into whether there is a smarter way that doesn't require the swap request id to be passed. TBD.
albert-llimos
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request
Closes: PRO-1962, PRO-1967
Checklist
Please conduct a thorough self-review before opening the PR.
Summary
Solana API and transaction builder use Versioned Transaction.
All uses of legacy transactions are replaced with versioned transactions.
Put in placeholders and TODOs to interface with other parts of the Versioned transaction work.