-
Notifications
You must be signed in to change notification settings - Fork 132
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
Claim funds on Substrate chain by providing proof of funds locking on PoA chain #91
Conversation
/// Verify that transaction is included into given finalized block. | ||
pub fn verify_transaction_finalized<S: Storage>( | ||
storage: &S, | ||
block: H256, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
block: H256, | |
block_hash: H256, |
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
…ity-bridges-common into ethereum-exchange
} | ||
|
||
// check if header is actually finalized | ||
let is_finalized = match header.number < finalized_number { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clippy would be mad if I didn't say anything :P
https://rust-lang.github.io/rust-clippy/master/index.html#match_bool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny nits left over, but other than that it looks good!
modules/currency-exchange/src/lib.rs
Outdated
T::DepositInto::deposit_into(recipient, amount).map_err(Error::<T>::from)?; | ||
|
||
// remember that we have accepted this transfer | ||
Transfers::<T>::insert(transfer_id, ()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking about this when reviewing earlier, but I think the code is correct. To make it more future proof I'd suggest to group the two statements that are related into one block, like so:
let recipient = T::RecipientsMap::map(transaction.recipient).map_err(Error::<T>::from)?;
let amount = T::CurrencyConverter::convert(transaction.amount).map_err(Error::<T>::from)?;
// make sure to update the mapping if we deposit successfully to avoid double spending,
// i.e. whenever `deposit_into` is successful we MUST update `Transfers`.
{
T::DepositInto::deposit_into(recipient, amount).map_err(Error::<T>::from)?;
Transfers::<T>::insert(transfer_id, ())
}
This will make it harder to introduce ?
statements in between deposit_into
and Transfers::insert
.
One last problem that affects
I've implemented (1) option here by moving pruning strategy from What needs to be done in long term? I'm not sure. Option (2) seems almost ideal, but I remember that Tomek has mentioned CHTs in some other context. So if you have any thoughts on that - please share, so that we may file an issue about that. |
I feel that claims on unfinalized blocks should be more expensive (in terms of weight cost), even exponentially expensive the more claims for unfinalized block there is. Otherwise we are risking claim spam on a block that potentially (or even most likely) is a fork block, but simply finality has stalled for a bit. Also this complicates cost of finalization quite a bit, cause now finalizing a header would need to perform the transfers. We could possibly do it lazily in I initially thought about option (3) as well, but without CHT - just storing So overall, let's start with Option 1, and have both Options 2 & 3 logged as possible "nice to have" improvements. |
I assume that the discussions about fixing interaction between the |
Yeah, thanks. I'll open PR and submit enhancement-issue once finality-cache PR is in. TODO for myself - I think option (1) should have been described and implemented slightly differently. Instead of counting ~24hrs (20000 blocks) from best finalized PoA block, known to Substrate, we should leave (unpruned) every finalized PoA header in Substrate for at least ~24hrs. Example of why it is required:
So the claimant has 0 possibility to present tx proof. And guarantees are not working. The biggest problem is that we don't want to leave headers for 24hrs when we're still syncing. So probably rely on PoA timestamp (TBD)? UPD: otoh, we do not want bridge to sync chain from the scratch, so disregard my note about "still syncing". |
Let's introduce an "admin" method to enabled/disable claims - that will allow us to prevent even a situation like this. |
… PoA chain (paritytech#91) * ethereum exchange module * continue * continue * added tests for exchange module * moved * remove println * move again * fixes * removed redundant deps * cargo fmt * fund_locks_transaction_decode_works * cargo fmt --all * fix error processing * added some tracing to bridge modules * more tests * more tests * cargo fmt --all * kovan.rs -> exchange.rs * Update bin/node/runtime/src/exchange.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * added assumption doc * Airdrop -> DepositInto * AsIs -> Identity * OnTransactionSubmitted * Transfers::Key = Id * typo * Update bin/node/runtime/src/exchange.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * block+tx+proof -> proof { block, tx, proof } * cargo fmt --all * docs * check <-> verify * parse hex * extracted exchange primitives to separate crate * added docs to runtime::exchange module * Update bin/node/runtime/src/exchange.rs Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> * typo * Update modules/currency-exchange/Cargo.toml Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> * add docs to currency-exchange module * change tests names * cargo fmt --all * Update bin/node/runtime/src/exchange.rs Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> * Update bin/node/runtime/src/exchange.rs Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> * Update bin/node/runtime/src/exchange.rs Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> * Update bin/node/runtime/src/exchange.rs Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> * Update bin/node/runtime/src/exchange.rs Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> * fixed verify_transaction_finalized for siblings of finalized blocks * cargo fmt --all * added double spend note * cargo fmt --all Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
… PoA chain (paritytech#91) * ethereum exchange module * continue * continue * added tests for exchange module * moved * remove println * move again * fixes * removed redundant deps * cargo fmt * fund_locks_transaction_decode_works * cargo fmt --all * fix error processing * added some tracing to bridge modules * more tests * more tests * cargo fmt --all * kovan.rs -> exchange.rs * Update bin/node/runtime/src/exchange.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * added assumption doc * Airdrop -> DepositInto * AsIs -> Identity * OnTransactionSubmitted * Transfers::Key = Id * typo * Update bin/node/runtime/src/exchange.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * block+tx+proof -> proof { block, tx, proof } * cargo fmt --all * docs * check <-> verify * parse hex * extracted exchange primitives to separate crate * added docs to runtime::exchange module * Update bin/node/runtime/src/exchange.rs Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> * typo * Update modules/currency-exchange/Cargo.toml Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> * add docs to currency-exchange module * change tests names * cargo fmt --all * Update bin/node/runtime/src/exchange.rs Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> * Update bin/node/runtime/src/exchange.rs Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> * Update bin/node/runtime/src/exchange.rs Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> * Update bin/node/runtime/src/exchange.rs Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> * Update bin/node/runtime/src/exchange.rs Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> * fixed verify_transaction_finalized for siblings of finalized blocks * cargo fmt --all * added double spend note * cargo fmt --all Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
So there's new module -
modules/currency-exchange
, which has one entry point:It checks whether provided tx is known to the 'bridge' module and its block is finalized, then it 'parses' that tx (for PoA -> Substrate transactions that means decoding
sender
,to
,nonce
,value
anddata
fields + verifies that it is the tx sent to pre-configured address) + maps from this transaction to local Substrate chain receiver (in current impl, the tx mush have encoded Substrate' account id in itsdata
field, as Tomek has suggested) + maps between currencies (currently 1:1) + grants given amount to given receiver.