feat: Open/Close Solana contract swap accounts #5321

ramizhasan111 · 2024-10-08T14:55:34Z

Pull Request

Checklist

Please conduct a thorough self-review before opening the PR.

I am confident that the code works.
I have written sufficient tests.
I have written and tested required migrations.
I have updated documentation where appropriate.

Summary

Please include a succinct description of the purpose and content of the PR. What problem does it solve, and how? Link issues, discussions, other PRs, and anything else that will help the reviewer.

Non-Breaking changes

If this PR includes non-breaking changes, select the non-breaking label. On merge, CI will automatically cherry-pick the commit to a PR against the release branch.

state-chain/pallets/cf-environment/src/lib.rs

foreign-chains/solana/sol-prim/src/consts.rs

dandanlen · 2024-10-15T09:58:36Z

state-chain/pallets/cf-environment/src/lib.rs

@@ -228,6 +242,21 @@ pub mod pallet {
 	#[pallet::getter(fn solana_api_environment)]
 	pub type SolanaApiEnvironment<T> = StorageValue<_, SolApiEnvironment, ValueQuery>;

+	#[pallet::storage]
+	#[pallet::getter(fn solana_open_contract_swap_accounts)]


nit: no need to add the getters, they aren't used and I think are now considered deprecated.

dandanlen · 2024-10-15T10:00:33Z

state-chain/pallets/cf-environment/src/lib.rs

+	#[pallet::storage]
+	#[pallet::getter(fn solana_closed_contract_swap_accounts)]
+	pub type SolanaClosedContractSwapAccounts<T> =
+		StorageMap<_, Blake2_128Concat, ContractSwapAccountAndSender, ()>;


Suggested change

StorageMap<_, Blake2_128Concat, ContractSwapAccountAndSender, ()>;

StorageMap<_, Twox64_Concat, ContractSwapAccountAndSender, ()>;

Simple hasher is enough here.

But I don't understand why we need to store this as a map at all - we never need to look up a single account we always remove them in batches.

I figured that we might have to close them one by one in some cases but I see that they will always be closed in batches. Will change to storage value.

dandanlen · 2024-10-15T10:01:10Z

state-chain/pallets/cf-environment/src/lib.rs

@@ -272,6 +342,8 @@ pub mod pallet {
 		StaleUtxosDiscarded { utxos: Vec<Utxo> },
 		/// Solana durable nonce is updated to a new nonce for the corresponding nonce account.
 		DurableNonceSetForAccount { nonce_account: SolAddress, durable_nonce: SolHash },
+		/// apicall to close Solana Contract Swap Accounts has been initiated.


Suggested change

/// apicall to close Solana Contract Swap Accounts has been initiated.

/// Transaction broadcast to close Solana Contract Swap Accounts has been initiated.

dandanlen · 2024-10-15T10:06:40Z

state-chain/pallets/cf-environment/src/lib.rs

@@ -245,6 +274,47 @@ pub mod pallet {
 	/// Contains the network environment for this runtime.
 	pub type ChainflipNetworkEnvironment<T> = StorageValue<_, NetworkEnvironment, ValueQuery>;

+	#[pallet::hooks]
+	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
+		fn on_idle(block_number: BlockNumberFor<T>, _remaining_weight: Weight) -> Weight {


We're using on_idle but we're not doing any weight calculations. If we don't care about weight (usually because we want to do something essential) then we can use on_finalize. If we want semantics of "only do this if we have enough weight to spare", then we should use on_idle, but we need to measure the weight.

yeah, I left the benchmarking for later. I'll open an issue for it.

Please do it in this PR - it's too easy to just kick all the 'boring parts' into a Linear ticket.

the benchmarking ticket could have been assigned to me and I could've taken it up immediately after this (and the election ticket) but sure I can do it as part of this PR if you insist.

I will come back to it after working on election ticket. Since that is closely linked to the functionality in this PR, some of the functionality in this PR might change and so I would do the benchmarks once the functionality is final.

dandanlen · 2024-10-15T10:09:01Z

state-chain/pallets/cf-environment/src/lib.rs

+	#[pallet::storage]
+	#[pallet::getter(fn solana_last_closed_contract_swap_accounts_at)]
+	pub type SolanaLastClosedContractSwapAccountsAt<T> =
+		StorageValue<_, BlockNumberFor<T>, OptionQuery>;


This can be ValueQuery (by default we last closed at block 0) and then we can simplify the logic - no need to check for None case.

I used Optionquery since it makes more conceptual sense. If there was no last apicall created, the value doesnt exist.

dandanlen · 2024-10-15T10:17:27Z

state-chain/pallets/cf-environment/src/lib.rs

@@ -272,6 +342,8 @@ pub mod pallet {
 		StaleUtxosDiscarded { utxos: Vec<Utxo> },
 		/// Solana durable nonce is updated to a new nonce for the corresponding nonce account.
 		DurableNonceSetForAccount { nonce_account: SolAddress, durable_nonce: SolHash },
+		/// apicall to close Solana Contract Swap Accounts has been initiated.
+		SolanaCloseContractSwapAccounts { broadcast_id: BroadcastId },


The event could be more informative... why not add the details of the account being closed?

state-chain/pallets/cf-environment/src/lib.rs

dandanlen · 2024-10-15T11:06:57Z

state-chain/pallets/cf-environment/src/lib.rs

+	pub type SolanaOpenContractSwapAccounts<T> =
+		StorageValue<_, Vec<ContractSwapAccountAndSender>, ValueQuery>;


I think it's likely that this will be stored in the election that witnesses these.

I think the election pallet would store the new accounts (the ones not stored here stored here) which it will then report. The SC (here) stores the accounts that it already knows about. Depends of course also on how the witnessing is implemented.

The witnessing needs to have access to this list, otherwise we will be querying like crazy and we want to avoid that. So I'm guessing that means it should be store in the election?! This information however is also needed here to know when to close accounts.
(I don't know enough details about the elections, just adding some info here)

I had also discussed this with @kylezs so either of you would take that task (PRO-1521). He can chip in for this discussion.

Doesn't have to be solved here and now. My understanding was that the election pallet will maintain a list of the currently opened accounts: it basically tries to sync this state between Solana and the SC. The engine will vote on accounts that are opened and closed.

So the election pallet will be the source of truth for OPEN accounts.
The enviroment can track accounts that are CLOSING.
It needs periodically pull the set of open accounts and diff this with CLOSING:

accounts that are OPEN and not CLOSING need to be closed

accounts that are CLOSING and not OPEN can be removed from the set of CLOSING accounts.

accounts that are CLOSING and OPEN can be ignored: we have already requested closure.

(But I get that the election side of things isn't implemented yet, so we need some placeholder for now.)

dandanlen · 2024-10-15T11:21:31Z

state-chain/runtime/src/chainflip.rs

+		tx_hash: TransactionHash,
+	) {
+		let _ = SolanaIngressEgress::contract_swap_request(
+			RuntimeOrigin::root(),


I think you can do something like this:

Suggested change

RuntimeOrigin::root(),

pallet_cf_witnesser::Origin::CurrentEpochWitnessThreshold.into(),

ramizhasan111 · 2024-10-21T13:09:24Z

This PR is on hold for now since I am currently working on the elections for Solana contract swap accounts tracking as the functionality in this PR might change based on how the elections are implemented. I will come back to this PR and finalize it after the implementing the elections.

albert-llimos · 2024-11-05T09:23:27Z

@ramizhasan111 Can this be closed? All the functionality is now part of #5355, right?

ramizhasan111 · 2024-11-05T09:43:17Z

@ramizhasan111 Can this be closed? All the functionality is now part of #5355, right?

yes, this PR is not needed anymore and can be closed.

ramizhasan111 · 2024-11-05T09:43:57Z

closing since it is not needed as the functionality is now part of #5355.

wip

9d83bde

ramizhasan111 changed the base branch from main to feat/solana-program-swaps-close-accounts October 8, 2024 14:55

feat: solana open/close contract swap accounts

b0c5f6c

ramizhasan111 changed the title ~~chore: adding api call~~ Open/Close Solana contract swap accounts Oct 11, 2024

feat: initiate contract swap

b0e82d0

ramizhasan111 requested a review from albert-llimos October 11, 2024 14:25

ramizhasan111 marked this pull request as ready for review October 11, 2024 15:04

ramizhasan111 requested a review from dandanlen as a code owner October 11, 2024 15:04

albert-llimos reviewed Oct 14, 2024

View reviewed changes

state-chain/pallets/cf-environment/src/lib.rs Outdated Show resolved Hide resolved

state-chain/pallets/cf-environment/src/lib.rs Show resolved Hide resolved

ramizhasan111 added 2 commits October 14, 2024 14:43

feat: root origin for contract_swap_request

e323c72

chore: address comment

c505b3c

albert-llimos reviewed Oct 14, 2024

View reviewed changes

foreign-chains/solana/sol-prim/src/consts.rs Show resolved Hide resolved

dandanlen reviewed Oct 15, 2024

View reviewed changes

dandanlen changed the title ~~Open/Close Solana contract swap accounts~~ feat: Open/Close Solana contract swap accounts Oct 21, 2024

ramizhasan111 mentioned this pull request Oct 25, 2024

feat: Solana Vault Swaps Elections #5355

Merged

4 tasks

ramizhasan111 closed this Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Open/Close Solana contract swap accounts #5321

feat: Open/Close Solana contract swap accounts #5321

ramizhasan111 commented Oct 8, 2024 •

edited

Loading

dandanlen Oct 15, 2024

dandanlen Oct 15, 2024

dandanlen Oct 15, 2024

ramizhasan111 Oct 16, 2024

dandanlen Oct 15, 2024

dandanlen Oct 15, 2024

ramizhasan111 Oct 16, 2024

dandanlen Oct 21, 2024

ramizhasan111 Oct 21, 2024

dandanlen Oct 15, 2024

ramizhasan111 Oct 15, 2024

dandanlen Oct 15, 2024

dandanlen Oct 15, 2024

ramizhasan111 Oct 15, 2024 •

edited

Loading

albert-llimos Oct 15, 2024

albert-llimos Oct 15, 2024

dandanlen Oct 16, 2024 •

edited

Loading

dandanlen Oct 15, 2024 •

edited

Loading

ramizhasan111 commented Oct 21, 2024

albert-llimos commented Nov 5, 2024

ramizhasan111 commented Nov 5, 2024

ramizhasan111 commented Nov 5, 2024

	StorageMap<_, Blake2_128Concat, ContractSwapAccountAndSender, ()>;
	StorageMap<_, Twox64_Concat, ContractSwapAccountAndSender, ()>;

	/// apicall to close Solana Contract Swap Accounts has been initiated.
	/// Transaction broadcast to close Solana Contract Swap Accounts has been initiated.

		pub type SolanaOpenContractSwapAccounts<T> =
		StorageValue<_, Vec<ContractSwapAccountAndSender>, ValueQuery>;

	RuntimeOrigin::root(),
	pallet_cf_witnesser::Origin::CurrentEpochWitnessThreshold.into(),

feat: Open/Close Solana contract swap accounts #5321

feat: Open/Close Solana contract swap accounts #5321

Conversation

ramizhasan111 commented Oct 8, 2024 • edited Loading

Pull Request

Checklist

Summary

Non-Breaking changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramizhasan111 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandanlen Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

dandanlen Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

ramizhasan111 commented Oct 21, 2024

albert-llimos commented Nov 5, 2024

ramizhasan111 commented Nov 5, 2024

ramizhasan111 commented Nov 5, 2024

ramizhasan111 commented Oct 8, 2024 •

edited

Loading

ramizhasan111 Oct 15, 2024 •

edited

Loading

dandanlen Oct 16, 2024 •

edited

Loading

dandanlen Oct 15, 2024 •

edited

Loading