Fix: Adding source to burnTransaction type #888

sameh-farouk · 2023-11-07T15:44:03Z

Description:

This PR fixes a bug in the tfchain bridge that cause a transfer to lost when below conditions met:

A swap from tfchain to stellar initiated during a bridge downtime
The target address on stellar don't have tft trust lines.

Changes:

Adding the source account ID to BurnTransaction type and to BurnTransactionExpired
Adjusting the BurnTransactionExpired event handler in the bridge daemon to enable refund to take place in a case that wasn't covered before.
Updating tfchain go client types.
Adding migration module to tft-bridge pallet and storage migration to v2

Why are these changes needed?

To be able to refund the transaction in some cases when the transfer can not complete in stellar side. see Unrecoverable bridge transfer #883
Also for audibility.

Related Issues:

Fixes Unrecoverable bridge transfer #883

Checklist:

My change requires a change to the documentation and I have updated it accordingly
My change requires storage migration and I have included and tested it following fork off and try_runtime instructions.
I have added tests to cover my changes.
My commits follow this conventional commits guide.

sameh-farouk · 2023-11-13T08:54:35Z

Migrations was tested on Devnet live data using try-runtime

try-runtime     --runtime ./tfchain_runtime_fix.compact.compressed.wasm     on-runtime-upgrade     live --uri ws://10.10.0.151:9944
[2023-11-13T02:18:56Z INFO  remote-ext] replacing ws:// in uri with http://: "http://10.10.0.151:9944" (ws is currently unstable for fetching remote storage, for more see https://github.com/paritytech/jsonrpsee/issues/1086)
[2023-11-13T02:18:56Z INFO  remote-ext] since no at is provided, setting it to latest finalized head, 0x7a7ab1e5c925f03e2e1ca75d871f1bf64841c34a3ce22256f1696b9e8a698edf
[2023-11-13T02:18:56Z INFO  remote-ext] since no prefix is filtered, the data for all pallets will be downloaded
[2023-11-13T02:18:56Z INFO  remote-ext] scraping key-pairs from remote at block height 0x7a7ab1e5c925f03e2e1ca75d871f1bf64841c34a3ce22256f1696b9e8a698edf
✅ Found 50739 keys (17.71s)
[00:00:53] ✅ Downloaded key values 955.2937/s [=====================================================================================================] 50739/50739 (0s)
✅ Inserted keys into DB (0.19s)
[2023-11-13T02:20:07Z INFO  remote-ext] adding data for hashed prefix: , took 71.16s
[2023-11-13T02:20:07Z INFO  remote-ext] adding data for hashed key: 3a636f6465
[2023-11-13T02:20:08Z INFO  remote-ext] adding data for hashed key: 26aa394eea5630e07c48ae0c9558cef7f9cce9c888469bb1a0dceaa129672ef8
[2023-11-13T02:20:08Z INFO  remote-ext] adding data for hashed key: 26aa394eea5630e07c48ae0c9558cef702a5c1b19ab7a04f536c519aca4983ac
[2023-11-13T02:20:08Z INFO  remote-ext] 👩‍👦 no child roots found to scrape
[2023-11-13T02:20:08Z INFO  remote-ext] initialized state externalities with storage root 0xe205b747e3eacc8c60d92841315cab617f1a429f8ffd388bbca352b921af7359 and state_version V0
[2023-11-13T02:20:09Z INFO  try-runtime::cli] Original runtime [Name: RuntimeString::Owned("substrate-threefold")] [Version: 146] [Code hash: 0xba30...d530]
[2023-11-13T02:20:10Z INFO  try-runtime::cli] New runtime      [Name: RuntimeString::Owned("substrate-threefold")] [Version: 146] [Code hash: 0x88cf...8fba]
[2023-11-13T02:20:10Z WARN  try-runtime::cli] New runtime spec version is not greater than the on-chain runtime spec version. Don't forget to increment the spec version if you intend to use the new code in a runtime upgrade.
[2023-11-13T02:20:11Z INFO  try-runtime::cli] 🚀 Speed up your workflow by using snapshots instead of live state. See `try-runtime create-snapshot --help`.
[2023-11-13T02:20:11Z INFO  try_runtime_core::commands::on_runtime_upgrade] 🔬 Running TryRuntime_on_runtime_upgrade with checks: PreAndPost
[2023-11-13T02:20:11Z INFO  pallet_tfgrid::migrations::v17] current pallet version: V17Struct
[2023-11-13T02:20:11Z INFO  pallet_tfgrid::migrations::v17] 🔎 FixFarmPublicIps pre migration: Number of existing farms 4374
[2023-11-13T02:20:11Z INFO  pallet_tfgrid::migrations::v17] 👥  TFGrid pallet to V17 passes PRE migrate checks ✅
[2023-11-13T02:20:11Z INFO  pallet_tfgrid::migrations::v17]  >>> Unused TFGrid pallet V17 migration
[2023-11-13T02:20:11Z INFO  pallet_tfgrid::migrations::v17] current pallet version: V17Struct
[2023-11-13T02:20:11Z INFO  pallet_tfgrid::migrations::v17] 👥  TFGrid pallet migration to V17Struct passes POST migrate checks ✅
[2023-11-13T02:20:11Z INFO  pallet_tft_bridge::migrations::v2] current pallet version: V1
[2023-11-13T02:20:11Z INFO  pallet_tft_bridge::migrations::v2] 🔎 MigrateBurnTransactionsV2 pre migration: Number of existing burn transactions 0
[2023-11-13T02:20:11Z INFO  pallet_tft_bridge::migrations::v2] 🔎 MigrateBurnTransactionsV2 pre migration: Number of existing executed burn transactions 351
[2023-11-13T02:20:11Z INFO  pallet_tft_bridge::migrations::v2] 👥  TFT-BRIDGE pallet to V1 passes PRE migrate checks ✅
[2023-11-13T02:20:11Z INFO  pallet_tft_bridge::migrations::v2]  >>> Migrating burn transactions storage...
[2023-11-13T02:20:11Z INFO  pallet_tft_bridge::migrations::v2]  <<< burnTransactions migration success, storage version upgraded
[2023-11-13T02:20:11Z INFO  pallet_tft_bridge::migrations::v2] current pallet version: V2
[2023-11-13T02:20:11Z INFO  pallet_tft_bridge::migrations::v2] 🔎 MigrateBurnTransactionsV2 post migration: Number of existing burn transactions 0
[2023-11-13T02:20:11Z INFO  pallet_tft_bridge::migrations::v2] 🔎 MigrateBurnTransactionsV2 post migration: Number of existing executed burn transactions 351
[2023-11-13T02:20:11Z INFO  try_runtime_core::commands::on_runtime_upgrade] 🔬 TryRuntime_on_runtime_upgrade succeeded! Running it again without checks for weight measurements.
[2023-11-13T02:20:11Z INFO  pallet_tfgrid::migrations::v17]  >>> Unused TFGrid pallet V17 migration
[2023-11-13T02:20:11Z INFO  pallet_tft_bridge::migrations::v2]  >>> Migrating burn transactions storage...
[2023-11-13T02:20:11Z INFO  pallet_tft_bridge::migrations::v2]  <<< burnTransactions migration success, storage version upgraded
[2023-11-13T02:20:11Z INFO  try_runtime_core::commands::on_runtime_upgrade] 🔬 Running TryRuntime_on_runtime_upgrade again to check idempotency: PreAndPost
[2023-11-13T02:20:11Z INFO  pallet_tfgrid::migrations::v17] current pallet version: V17Struct
[2023-11-13T02:20:12Z INFO  pallet_tfgrid::migrations::v17] 🔎 FixFarmPublicIps pre migration: Number of existing farms 4374
[2023-11-13T02:20:12Z INFO  pallet_tfgrid::migrations::v17] 👥  TFGrid pallet to V17 passes PRE migrate checks ✅
[2023-11-13T02:20:12Z INFO  pallet_tfgrid::migrations::v17]  >>> Unused TFGrid pallet V17 migration
[2023-11-13T02:20:12Z INFO  pallet_tfgrid::migrations::v17] current pallet version: V17Struct
[2023-11-13T02:20:12Z INFO  pallet_tfgrid::migrations::v17] 👥  TFGrid pallet migration to V17Struct passes POST migrate checks ✅
[2023-11-13T02:20:12Z INFO  pallet_tft_bridge::migrations::v2] current pallet version: V2
[2023-11-13T02:20:12Z INFO  pallet_tft_bridge::migrations::v2]  >>> Unused TFT-BRIDGE pallet V2 migration
[2023-11-13T02:20:12Z INFO  pallet_tft_bridge::migrations::v2] current pallet version: V2
[2023-11-13T02:20:12Z INFO  pallet_tft_bridge::migrations::v2] 🔎 MigrateBurnTransactionsV2 post migration: Number of existing burn transactions 0
[2023-11-13T02:20:12Z INFO  pallet_tft_bridge::migrations::v2] 🔎 MigrateBurnTransactionsV2 post migration: Number of existing executed burn transactions 351
[2023-11-13T02:20:12Z INFO  try-runtime::cli] PoV size (zstd-compressed compact proof): 61.0 KB. For parachains, it's your responsibility to verify that a PoV of this size fits within any relaychain constraints.
[2023-11-13T02:20:12Z INFO  try-runtime::cli] Consumed ref_time: 0.043975s (2.20% of max 2s)
[2023-11-13T02:20:12Z INFO  try-runtime::cli] ✅ No weight safety issues detected. Please note this does not guarantee a successful runtime upgrade. Always test your runtime upgrade with recent state, and ensure that the weight usage of your migrations will not drastically differ between testing and actual on-chain execution.

substrate-node/pallets/pallet-tft-bridge/src/benchmarking.rs

substrate-node/pallets/pallet-tft-bridge/src/lib.rs

renauter

Nice work!!
Added some suggestions
Should also join ADR file to describe changes
Tested on all networks and worked 🎉

substrate-node/runtime/src/lib.rs

substrate-node/pallets/pallet-tft-bridge/src/types.rs

substrate-node/pallets/pallet-tft-bridge/src/lib.rs

substrate-node/pallets/pallet-tft-bridge/src/migrations/v2.rs

substrate-node/pallets/pallet-tft-bridge/src/migrations/types.rs

substrate-node/pallets/pallet-tft-bridge/src/migrations/v2.rs

bridge/tfchain_bridge/pkg/substrate/events.go

sameh-farouk · 2023-11-14T16:11:29Z

@renauter ADR file added and all comments was resolved

renauter · 2023-11-14T19:10:46Z

@renauter ADR file added and all comments was resolved

Great! All good, you can "Squash and merge" ;)

sameh-farouk added 8 commits November 6, 2023 02:36

fix: add source account to burnTransaction type

2d22254

feat: pallet-tft-bridge migration to v2

06fe47d

fix: fix storage ver and pre/post migration check

48f1c8e

feat: trigger migration

23891fd

update types in go client

f6cd8c8

fix: allow refund on BurnTransactionExpired handle

9b201be

fix: fix types in go client

f2a4c66

style: apply gofmt

abcae89

sameh-farouk marked this pull request as ready for review November 12, 2023 09:29

sameh-farouk requested review from renauter and LeeSmet as code owners November 12, 2023 09:29

test: fix try-runtime pre and post hooks

efb6e07

style: apply rustfmt

2c39455

muhamadazmy approved these changes Nov 13, 2023

View reviewed changes

substrate-node/pallets/pallet-tft-bridge/src/benchmarking.rs Show resolved Hide resolved

substrate-node/pallets/pallet-tft-bridge/src/lib.rs Outdated Show resolved Hide resolved

refactor: remove redundant import

7a413a0

renauter reviewed Nov 13, 2023

View reviewed changes

bridge/tfchain_bridge/pkg/substrate/events.go Outdated Show resolved Hide resolved

sameh-farouk added 5 commits November 14, 2023 15:13

refactor: remove redundant imports

609ccc7

Fix: adjust source type for WithdrawExpiredEvent

443275d

style: apply gofmt

dd358c6

refactor: Added storage alias for v2 types

6909f12

doc: adding ADR file describe changes

32cf695

doc: update ADR

1c48c34

sameh-farouk merged commit ab138c8 into development Nov 14, 2023
2 checks passed

sameh-farouk deleted the development-fix-883-add-source-to-burnTransaction-type branch November 14, 2023 19:22

sameh-farouk mentioned this pull request Nov 21, 2023

Update types threefoldtech/tfchain_graphql#140

Closed

sameh-farouk mentioned this pull request Dec 27, 2023

The client has changes for the incoming tfchain runtime 147 which is not upgraded yet on live networks threefoldtech/tfgrid-sdk-go#576

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Adding source to burnTransaction type #888

Fix: Adding source to burnTransaction type #888

sameh-farouk commented Nov 7, 2023 •

edited

Loading

sameh-farouk commented Nov 13, 2023

renauter left a comment

sameh-farouk commented Nov 14, 2023

renauter commented Nov 14, 2023

Fix: Adding source to burnTransaction type #888

Fix: Adding source to burnTransaction type #888

Conversation

sameh-farouk commented Nov 7, 2023 • edited Loading

Description:

Changes:

Why are these changes needed?

Related Issues:

Checklist:

sameh-farouk commented Nov 13, 2023

renauter left a comment

Choose a reason for hiding this comment

sameh-farouk commented Nov 14, 2023

renauter commented Nov 14, 2023

sameh-farouk commented Nov 7, 2023 •

edited

Loading