feat: Solana Vault Swaps Elections #5355
Conversation
ramizhasan111
changed the base branch from
main
to
feat/solana-program-swaps-close-accounts
ramizhasan111
changed the base branch from
feat/solana-program-swaps-close-accounts
to
feat/closing-solana-swap-accounts
ramizhasan111
changed the base branch from
feat/closing-solana-swap-accounts
to
feat/solana-program-swaps-close-accounts
state-chain/pallets/cf-elections/src/electoral_systems/solana_swap_accounts_tracking.rs
Outdated
Show resolved
Hide resolved
state-chain/pallets/cf-elections/src/electoral_systems/solana_swap_accounts_tracking.rs
Outdated
Show resolved
Hide resolved
| pub confirm_closed_accounts: BTreeSet<Account>, | ||
| } | ||
|
|
||
| pub struct SolanaVaultSwapAccounts< |
There was a problem hiding this comment.
wanted to first nail down the approach based on the reviews before writing any tests
| for vote in active_votes { | ||
| counts.entry(vote).and_modify(|count| *count += 1).or_insert(1); | ||
| } |
There was a problem hiding this comment.
will this actually work? If nodes are querying at different times then their votes may not be identical, and therefore wouldn't come to consensus
There was a problem hiding this comment.
I agree - we need to count each account addition/removal individually and then keep only the additions/removals that have consensus votes.
There was a problem hiding this comment.
I originally wanted to have engines vote on individual accounts but the way Bitmap Components work doesn't allow that (we cant vote for more than one bitmap component in a single vote). So I instead took the route of coming to consensus over the whole list. But I can see that it is possible we cant come to a consensus over the whole list. Will modify the consensus to parse the votes and count votes for individual accounts.
There was a problem hiding this comment.
The bitmap should generally only be used for when we expect identical votes
There was a problem hiding this comment.
updated the consensus.
state-chain/pallets/cf-elections/src/electoral_systems/solana_swap_accounts_tracking.rs
Outdated
Show resolved
Hide resolved
state-chain/pallets/cf-elections/src/electoral_systems/solana_swap_accounts_tracking.rs
Outdated
Show resolved
Hide resolved
| pub trait SolanaVaultSwapAccountsHook<Account, SwapDetails, E> { | ||
| fn close_accounts(accounts: Vec<Account>) -> Result<(), E>; | ||
| fn initiate_vault_swap(swap_details: SwapDetails); | ||
| fn get_number_of_available_sol_nonce_accounts() -> usize; | ||
| } |
There was a problem hiding this comment.
I don't think we should be coupling this so tightly to Solana.
Can we not follow the same patterns/conventions as we previously have, with the hooks defined externally rather than in the pallet?
There was a problem hiding this comment.
Since this is a solana specific Electoral system for a specific use-case, I dont see any problem having a custom hook.
Can we not follow the same patterns/conventions as we previously have, with the hooks defined externally rather than in the pallet?
not sure what you mean here? this follows the pattern we use in other electoral systems where we define a hook alongside the electoral system (which the electoral system uses to call into) and then implement the hook at the runtime level where we initialize the electoral system.
There was a problem hiding this comment.
What I mean is that this electoral system includes a lot of non-election/consensus related logic inside of itself. ie. the interfaces for other electoral systems is defined in term of hooks that trigger externally-defined logic. However here the 'hooks' are not really hooks: they are accessors to specific external functionality, and the logic for how to process those things is internal to the election. We're mixing contexts.
Rather than get_number_of_available_sol_nonce_accounts and initiate_vault_swap, I think it should just have a single hook that says something like on_account_change(added, removed); and the logic of what to do with that information should be defined externally.
There was a problem hiding this comment.
Given that its a custom election which is not intended to be re-used, I dont see why we cant have some custom logic in it especially if that logic is closely tied to what the election is about. We can rename the trait to not indicate that it is a hook but a general trait that calls some external code. The idea of injecting a trait that provides external code access is general enough here and the ElectoralSystem trait doesnt force us that the trait injected necessarily has to fit the definition of a hook (whatever that might be).
Even if we move out some of the functionality we would then need that logic to return some information back to the electoral system which it then uses to create a new election which would still mean it would still not function like a hook. This is just the nature of this electoral system use case.
In other electoral systems as well the code in on_finalize of the ElectoralSystem includes logic of first checking for consensus (calling the check_consensus) and then executing some logic to then decide whether to start a new election and with what properties. It is also the same here with just some external logic accessed through the Hook.
There was a problem hiding this comment.
I don't want to spend days arguing back and forth about this. In general I would prefer if we follow existing patterns and conventions when possible, that's all. It makes maintenance easier, testing easier, reusability easier. Please keep it in mind in future.
There are always exceptions and grey areas, but I don't think this needed to be an exception.
* chore: add get_event_accounts * feat: add get_program_swaps * chore: add get_program_swaps * chore: improvements * chore: also update persa.rs * chore: cleanup
…ana-vault-swaps-election
Pull Request
Closes: PRO-1521 , PRO-1522
Checklist
Please conduct a thorough self-review before opening the PR.
Summary
note: please ignore some of the naming around Contract Swaps. That will be fixed before merging.