Argo bridge runtime pallet

[kdembler]

See background in #5084

Goal

Develop argoBridge runtime pallet that will be used for Argo bridge operations.

Required functionalities:

• Allowing users to request bridging to Ethereum.
• Allowing bridge operators to finalise transfers from Ethereum.
• Proper security model as described in the background issue.
• Letting the Council adjust pallet configuration via a proposal.

Pallet

State

1. status: enum with options { type: 'active' } | { type: 'paused' } | { type: 'thawn'; thawn_started_at: 123456 }.
2. operator_account: account ID.
3. pauser_accounts: list of account IDs with permission to pause the bridge operations.
4. mint_allowance: number indicating number of tokens that the bridge pallet is able to mint. This should always equal total_burned - total_minted.
5. bridging_fee: amount of JOY burned as a fee for each transfer.
6. thawn_duration: number of blocks needed before bridge unpause can be finalised.

Proposal

Single 3/3 proposal allowing the Council to update the following state values:

1. operator_account
2. pauser_accounts
3. bridging_fee
4. thawn_duration

Emits BridgeConfigUpdated(config)event

Extrinsics

1. request_outbound_transfer(dest_account, amount, expected_fee) - callable by anybody:
   1. Checks that the status is { type: 'active' }.
   2. Burns the fee
   3. Burns amount of JOY from sender account, increases mint_allowance by amount
   4. Computes transfer_id by hashing (dest_account, amount, block_number, call_index_in_block)
   5. Emits an event OutboundTransferRequested(transfer_id, dest_account, amount, paid_fee)
2. finalize_inbound_transfer(dest_account, amount, request_block, request_nonce) - callable by operator_account only:
   1. Checks that the status is { type: 'active' }.
   2. Computes transfer_id by hashing (dest_account, amount, request_block, request_nonce)
   3. Checks that amount is not bigger than current mint_allowance
   4. Decreases mint_allowance by amount, mints amount of JOY tokens to dest_account
   5. Emits an event InboundTransferFinalized(transfer_id)
3. pause_bridge() - callable by any of pauser_accounts:
   1. Changes status to { type: 'paused' }
   2. Emits an event BridgePaused(pauser)
4. init_unpause_bridge() - callable by any of pauser_accounts:
   1. Changes status to { type: 'thawn'; thawn_started_at: current_block }
   2. Emits an event BridgeThawnStarted(pauser)
5. finish_unpause_bridge() - callable by operator_account only:
   1. Checks that status.type === 'thawn && current_block > status.thawn_started_at + thawn_duration`
   2. Changes status to { type: 'active' }
   3. Emits an event BridgeThawnFinished()

[freakstatic]

Emits BridfeConfigUpdated(config)event

@kdembler maybe you mean BridgeConfigUpdated?

[freakstatic]

@kdembler on request_outbound_transfer what is the meaning of the expected_fee?

[kdembler]

@freakstatic the goal is to make sure the user is aware of the current fee. Without it this could happen:

1. Users sees 50 JOY fee in the UI
2. They start bridging process
3. The fee gets updated in the meantime to 100 JOY
4. Users sends a tx expecting to pay 50 but they end up paying 100.

With expected_fee, the user can specify what they think the fee is, so if it's something else, the tx can be rejected.

[freakstatic]

@freakstatic the goal is to make sure the user is aware of the current fee. Without it this could happen:

1. Users sees 50 JOY fee in the UI
2. They start bridging process
3. The fee gets updated in the meantime to 100 JOY
4. Users sends a tx expecting to pay 50 but they end up paying 100.

With expected_fee, the user can specify what they think the fee is, so if it's something else, the tx can be rejected.

Oh yeah that makes sense, should it be a exact match or should we defined an acceptable interval?

[freakstatic]

Another question, what should be the value of the call_index_in_block?

[kdembler]

Oh yeah that makes sense, should it be a exact match or should we defined an acceptable interval?

Should be exact, I don't expect this to happen often.

Another question, what should be the value of the call_index_in_block?

The goal here is to identify the extrinsic uniquely. If we use just a block number, then it would be possible for 2 bridging requests in a single block to have the exact same ID. My idea was to use the index of the extrinsic in the block to uniquely identify it. But I'm not sure if you have access to that in Substrate. If that's not available, we could use some kind of nonce counter that's incremented every time transfer is requested and would be included in the ID hash.

[freakstatic]

Oh yeah that makes sense, should it be a exact match or should we defined an acceptable interval?

Should be exact, I don't expect this to happen often.

Another question, what should be the value of the call_index_in_block?

The goal here is to identify the extrinsic uniquely. If we use just a block number, then it would be possible for 2 bridging requests in a single block to have the exact same ID. My idea was to use the index of the extrinsic in the block to uniquely identify it. But I'm not sure if you have access to that in Substrate. If that's not available, we could use some kind of nonce counter that's incremented every time transfer is requested and would be included in the ID hash.

I don't think it's possible to get the index extrinsic, so I think that the counter should do it.
But this value will need to be know when calling the finalize_inbound_transfer no?
This raises another question which is the current use of the request_nonce 🤔
Probably was already intended for the value of call_index_in_block right?

[freakstatic]

Notes from the meeting on 23/04/2024:

• MaxPauserAccounts: 5
• request_outbound_transfer
  • dest_account -> vector of bytes to be passed to the ETH side
  • add request counter to make requests transfer_id's unique
• to generate the transfer_id it's necessary to use some of this algorithms (keccak256, ripemd160 or sha256)

[kdembler]

@freakstatic here's my suggestion for transfer declarations after our recent discussions. Initially I was thinking about protobuf descriptor for remote dest_account, but after more thinking and prototyping I believe it's best to declare those structs directly in the pallet. This way we don't need to serialize/deserialize and make it more transparent. We also need some descriptor for remote chains for finalization without hashing. I added more fields to the InboundTransferFinalized event so even Joystream-only indexers can track minting. Let me know what you think.

// Chain ID as defined by some Joystream chain registry. We should use EIP-155 for EVM chains.
pub type ChainId = u32;

// Incremental ID for transfers
pub type TransferId = u64;

pub struct RemoteAccount {
    // Ethereum addresses only need 20 bytes but we use 32 bytes to enable compatibility with other chains
    // When using Ethereum addresses, the last 12 bytes should be zero
    pub account: [u8; 32],
    pub chain_id: ChainId,
}

pub struct RemoteTransfer {
    pub id: TransferId,
    pub chain_id: ChainId,
}

pub enum Event {
    OutboundTransferRequested(
        TransferId,
        T::AccountId, // account requesting transfer
        RemoteAccount,
        BalanceOf<T>, // amount being transferred (burned)
        BalanceOf<T>, // paid fee (burned)
    ),
    InboundTransferFinalized(
        RemoteTransfer,
        T::AccountId, // account to which new tokens were minted
        BalanceOf<T>, // amount minted
    ),
}

// Request an outbound transfer to a remote chain
// Burns native JOY so that it can be minted on the remote chain
// dest_account - account on the remote chain to which the transfer is being made
// amount - amount of JOY being transferred, to be burned from the caller account
// expected_fee - expected fee for the transfer, to be burned from the caller account
pub fn request_outbound_transfer(dest_account: RemoteAccount, amount: BalanceOf<T>, expected_fee: BalanceOf<T>)

// Finalize an inbound transfer from a remote chain
// Mints native JOY that was burned on the remote chain
// remote_transfer - descriptor of the remote transfer being finalized
// dest_account - Joystream account to which native JOY is being minted
// amount - amount of JOY being minted
pub fn finalize_inbound_transfer(remote_transfer: RemoteTransfer, dest_account: T::AccountId, amount: BalanceOf<T>)

[freakstatic]

@freakstatic here's my suggestion for transfer declarations after our recent discussions. Initially I was thinking about protobuf descriptor for remote dest_account, but after more thinking and prototyping I believe it's best to declare those structs directly in the pallet. This way we don't need to serialize/deserialize and make it more transparent. We also need some descriptor for remote chains for finalization without hashing. I added more fields to the InboundTransferFinalized event so even Joystream-only indexers can track minting. Let me know what you think.

// Chain ID as defined by some Joystream chain registry. We should use EIP-155 for EVM chains.
pub type ChainId = u32;

// Incremental ID for transfers
pub type TransferId = u64;

pub struct RemoteAccount {
    // Ethereum addresses only need 20 bytes but we use 32 bytes to enable compatibility with other chains
    // When using Ethereum addresses, the last 12 bytes should be zero
    pub account: [u8; 32],
    pub chain_id: ChainId,
}

pub struct RemoteTransfer {
    pub id: TransferId,
    pub chain_id: ChainId,
}

pub enum Event {
    OutboundTransferRequested(
        TransferId,
        T::AccountId, // account requesting transfer
        RemoteAccount,
        BalanceOf<T>, // amount being transferred (burned)
        BalanceOf<T>, // paid fee (burned)
    ),
    InboundTransferFinalized(
        RemoteTransfer,
        T::AccountId, // account to which new tokens were minted
        BalanceOf<T>, // amount minted
    ),
}

// Request an outbound transfer to a remote chain
// Burns native JOY so that it can be minted on the remote chain
// dest_account - account on the remote chain to which the transfer is being made
// amount - amount of JOY being transferred, to be burned from the caller account
// expected_fee - expected fee for the transfer, to be burned from the caller account
pub fn request_outbound_transfer(dest_account: RemoteAccount, amount: BalanceOf<T>, expected_fee: BalanceOf<T>)

// Finalize an inbound transfer from a remote chain
// Mints native JOY that was burned on the remote chain
// remote_transfer - descriptor of the remote transfer being finalized
// dest_account - Joystream account to which native JOY is being minted
// amount - amount of JOY being minted
pub fn finalize_inbound_transfer(remote_transfer: RemoteTransfer, dest_account: T::AccountId, amount: BalanceOf<T>)

Looks good, yeah I think skipping protobuf serialization makes things simpler.
the chain_id would be 1 or 0 for eth mainnet then?

[bedeho]

Just a reminder to avoid adding structure to data that the runtime will not need to validate itself. I'm guessing the runtime does not need to know anything about chains or accounts on other chains to do its validation. Of course you can always add validation to the runtime, but one always ends up regretting it later because you will find some use case you did not think of. Like for example you will end up wanting to have two distinct assets on the same chain or something like that. So it may be better to truly leave that as pure metadata that the chain does not process, only indexers.

[kdembler]

@freakstatic

the chain_id would be 1 or 0 for eth mainnet then

Following EIP-155 Eth mainnet is 1.

@bedeho what is the cost of introducing a runtime struct? We have started with metadata blob but with the current design transfers are identified by (counter_id, chain_id) tuple. When finalizing remote transfer both of those need to be specified. If struct was really bad we could just do "counter_id-chain_id" string but a struct seems much more readable.

[bedeho]

what is the cost of introducing a runtime struct

That will be apparent at some point in the future, and the benefit now is nil. Consider, at one point in an early design, the runtime was aware of fields like video titles, descriptions, etc. As far as I can gather, this is doing the same thing.

struct seems much more readable.

Issue is runtime does not need to read it.

[kdembler]

@bedeho Okay, I understand why you believe runtime struct is not needed, but I still have no idea why that's something we potentially shouldn't do. Is there cost to introducing a type?

[bedeho]

Because in the future you will want it to be different in some way you now are not anticipating, as goals change, and changing the runtime is way more costly than changing the metadata standard.

[kdembler]

This risk I'm aware of and willing to take. If we ever need more functionality than account+chain then it will surely be discussed and thought about well in advance giving us time to adjust runtime if needed.

[bedeho]

What is the upside here, nothing that I can see. this also breaks a very well established norm of only putting what is absolutely required in the execution side of consensus, breaking this has only ever not worked out in my experience, such as storage pallet, which at least to begin with had a semi-plausible rationale for why to put it in consensus logic. Anyway, no need to reply to my post, but I think this would be a mistake and I want that to be noted.

[kdembler]

The upsides:

• We don't need a magic string standard for identifying transfers from remote chains (like "eth-1"), instead it's a struct with well-described chain_id and transfer_id fields
• No need to maintain the metadata standard outside of the runtime.
• No need to serialize/deseralize anything, making app logic easier
• You can inspect all transfers info directly from block explorer without using additional tools to interpret the data
• You can submit extrinsics directly through polkadot apps if needed without need to encode any data

The only downside I see is slightly lower flexibility if we need additional data in the future. In my opinion the upsides win