feat: fallback for vault swap if no broker provided #5507
Conversation
Janislav
force-pushed
the
feat/pro-1804/fallback-for-vault-swap
branch
from
54149de to
48dcf8e
Compare
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
Janislav
force-pushed
the
feat/pro-1804/fallback-for-vault-swap
branch
from
11c9efd to
f2ba32b
Compare
Janislav
changed the title
Janislav
changed the title
Janislav
force-pushed
the
feat/pro-1804/fallback-for-vault-swap
branch
from
22f89a1 to
e1840f6
Compare
Janislav
requested review from
dandanlen and
martin-chainflip
as code owners
| @@ -766,6 +766,18 @@ pub mod pallet { | |||
| pub type NetworkFeeDeductionFromBoostPercent<T: Config<I>, I: 'static = ()> = | |||
| StorageValue<_, Percent, ValueQuery>; | |||
|
|
|||
| /// Stores the tx_id of an aborted vault transaction. At the moment we consider a vault | |||
| /// transaction as aborted if | |||
There was a problem hiding this comment.
This comment is incomplete.
| /// transaction as aborted if | ||
| #[pallet::storage] | ||
| pub(crate) type AbortedVaultTransaction<T: Config<I>, I: 'static = ()> = | ||
| StorageMap<_, Identity, TransactionInIdFor<T, I>, ()>; |
There was a problem hiding this comment.
I don't think we can use Identity for the TransactionInId (we don't really have any guarantees about what it contains). TwoX64 hash should be enough though.
| /// Stores the details of an aborted vault transaction. In case we fail to refund we store the | ||
| /// details in this storage item. | ||
| #[pallet::storage] | ||
| pub(crate) type AbortedVaultTransactionDetails<T: Config<I>, I: 'static = ()> = |
There was a problem hiding this comment.
Do we need two separate storage items? Can we not just use this with an optional value?
There was a problem hiding this comment.
We can also unify it.
There was a problem hiding this comment.
EDIT: We can not unify it and it's better to keep it separate. We could even argue to just remove this and not save this, since this is a very theoretical case. After we enforced refund parameters, the only case a refund could fail is when fail to build an API call, but that should actually never happen.
There was a problem hiding this comment.
Yes exactly - we don't need the map with a () value.
| @@ -2161,6 +2193,12 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> { | |||
| boost_fee, | |||
| }: VaultDepositWitness<T, I>, | |||
| ) { | |||
| if Self::should_reject_vault_swap(&broker_fee) { | |||
There was a problem hiding this comment.
We only check this when we pre-witness. Why not on full deposits? What about chains where we don't have pre-witnessing - am I missing something?
There was a problem hiding this comment.
I think we should always have the prewitness step, right? The reason why we do it here is boost. Otherwise, we would first send out the funds and then later also refund it.
There was a problem hiding this comment.
Pretty sure we only prewitness for Bitcoin. Also, sometimes we miss the pre-witness (for example if there's a reorg).
There was a problem hiding this comment.
Okay, but checking it only on full deposit is not an option since it could be a boosted BTC transaction. We have to check it on both stages but only refund it on full deposit
- Skipping on prewitness stage and refund when we finalize the deposit - Removed obsolete storage items - Fixed tests
Pull Request
Closes: PRO-1804
Checklist
Please conduct a thorough self-review before opening the PR.
Summary
Adding refund functionality if there is no broker provided in the vault swap. If we identify on the prewitness stage that there is no broker provided or the account provided is no broker, we mark the tx as aborted. When the deposit gets eventually fully witnessed, we check if it was identified as aborted and directly try to refund it. If the refund should not be possible (failing to create the API call for e.x) we push the details of the tx to another storage item to remember the tx.
Non-Breaking changes
If this PR includes non-breaking changes, select the
non-breakinglabel. On merge, CI will automatically cherry-pick the commit to a PR against the release branch.