chainsafe / chainbridge-substrate Goto Github PK

Hey,
I need help regarding asset transfer. I want to transfer some other asset from Eth -> Substrate and also want to associate some other method with it. Could you help me what all I have to do for that?

To be compatible with existing chains we should avoid using genesis to configure the bridge params. They can default to zero, as long a methods are provided to configure them as root.

Use map-set for storing data that we only care about the key. The Relayers are tracked based on their account ID, which will be the key for Storage Set.

This would have an added benefit for the underlying database implementation to optimize the storage size since we are storing less data.

Implementation details

Basically, we change the value from bool type to () type which effectively makes the StorageMap a Set.

From

    /// Tracks current relayer set
    #[pallet::storage]
    #[pallet::getter(fn relayers)]
    pub type Relayers<T: Config> =
        StorageMap<_, Blake2_256, T::AccountId, bool, ValueQuery>;

To

    /// Tracks current relayer set
    #[pallet::storage]
    #[pallet::getter(fn relayers)]
    pub type Relayers<T: Config> =
        StorageMap<_, Blake2_256, T::AccountId, (), OptionQuery>;

and is_relayer function will just check if the Relayers storage contains the key of a certain AccountId.

        fn is_relayer(who: &T::AccountId) -> bool {
            Relayers::<T>::contains_key(who)
        }

Testing details

No changes needed to the existing tests

Acceptance Criteria

All existing test should pass as is.

We need an example pallet that can handler ERC721 tokens.

Expected Behavior

A runtime should build when this pallet is included. Issue relates to extra method required in EnsureOrigin that is enabled when the runtime-benchmarks feature is set.

Current Behavior

Build fails with error

error[E0046]: not all trait items implemented, missing: `successful_origin`
   --> /home/willem/.cargo/registry/src/github.com-1ecc6299db9ec823/frame-system-3.0.0/src/lib.rs:786:1
    |
786 | / impl<
787 | |     O: Into<Result<RawOrigin<AccountId>, O>> + From<RawOrigin<AccountId>>,
788 | |     AccountId,
789 | | > EnsureOrigin<O> for EnsureRoot<AccountId> {
...   |
801 | |     }
802 | | }
    | |_^ missing `successful_origin` in implementation
    |
    = help: implement the missing item: `fn successful_origin() -> OuterOrigin { todo!() }`

It is much harder to handle any kind of error in Substrate than it is to do it on Ethereum smart contracts. While on Ethereum one can simple use the revert opcode and be sure that all storage reverts to the previous state the same is not true for Subtrate. All the storage changes that occurred before this error will remain intact.

The best practice in Substrate is to follow the “verify first, write last” principle. During execution you should make all the checks are using the ensure macro and that when you start modifying storage, no error can be thrown after.

While the outlined approach sounds clean, it can get very complicated when inter-pallet communication is thrown into the mix. This is especially true when multiple different external pallets need be called in one function. In such cases it becomes extremely hard to do all the verifications before changing storage.
Examples

There are a few places where this problem may occur:

• transfer_fungible, transfer_nonfungible, transfer_generic functions of the Substrate library are called during the deposit from another pallet. They make use of the ensure macro, so they may potentially fail.

chainbridge/src/lib.rs:L487-L506

   pub fn transfer_fungible(
       dest_id: ChainId,
       resource_id: ResourceId,
       to: Vec<u8>,
       amount: U256,
   ) -> DispatchResult {
       ensure!(
           Self::chain_whitelisted(dest_id),
           Error::<T>::ChainNotWhitelisted
       );
       let nonce = Self::bump_nonce(dest_id);
       Self::deposit_event(RawEvent::FungibleTransfer(
           dest_id,
           nonce,
           resource_id,
           amount,
           to,
       ));
       Ok(())
   }

This means that these functions should be called either before any storage changes (outside of current pallet) or the same checks should be done in the outside pallet, before changing the state. Even though example pallets are out of scope, in this example you can see that the actual transfer is happening before calling the transfer_fungible function:

example-pallet/src/lib.rs:L72-L80

   pub fn transfer_native(origin, amount: BalanceOf<T>, recipient: Vec<u8>, dest_id: bridge::ChainId) -> DispatchResult {
       let source = ensure_signed(origin)?;
       ensure!(<bridge::Module<T>>::chain_whitelisted(dest_id), Error::<T>::InvalidTransfer);
       let bridge_id = <bridge::Module<T>>::account_id();
       T::Currency::transfer(&source, &bridge_id, amount.into(), AllowDeath)?;
  
       let resource_id = T::NativeTokenId::get();
       <bridge::Module<T>>::transfer_fungible(dest_id, resource_id, recipient, U256::from(amount.saturated_into()))
   }

That may lead to tokens actually being transferred to the Bridge, but the event to transfer them outside is not emitted.

• When a proposal passes, its state is saved to storage before calling finalize_execution function.

chainbridge/src/lib.rs:L421-L429

let complete = votes.try_to_complete(<RelayerThreshold>::get(), <RelayerCount>::get());
<Votes<T>>::insert(src_id, (nonce, prop.clone()), votes.clone());
Self::deposit_event(RawEvent::VoteFor(src_id, nonce, who.clone()));

if complete {
    Self::finalize_execution(src_id, nonce, prop)
} else {
    Ok(())
}

inside of finalize_execution function, an external pallet is called (this pallet is not implemented yet) which can potentially also fail, and even if that happens, the proposal will still be considered as executed.

In order to handle the case where multiple relayers attempt to submit a create_proposal extrinsic in the same block, we should consider creation and approval the same action.

This can be deprecated in favour of the moduleId account Id.

TBD: How can we allocate to this accountId in genesis?

Presently whitelist is only checked on transfer out. It would be trivial to also check when accepting a proposal and would likely be expected behaviour.

In order to support administration methods aside from sudo/root, we should add an (optional) origin to the chainbridge pallet trait. This allows a primary origin check to be provided, with root being the fallback.

Implementation details

This is to prevent u32/u64 overflow when values reach the minimum/maximum number

Testing details

Acceptance Criteria

Substrate does not have default overflow/underflow protection. In order to avoid this kind of bugs, it’s better to use safe functions like checked_add() and checked_sub(). In the current codebase math operations are mostly either incrementing (+1) or making operations with values that shouldn’t overflow/underflow logically:

chainbridge/src/lib.rs:L64

} else if self.votes_against.len() > (total - threshold) as usize {

It’s not really a big issue here, but if the threshold is bigger than the total, we’ll have underflow.

To distinguish this repo from other repositories that has similar names, it would be best to use a more descriptive name. At a glance we will have an idea that it is not a replacement code for the repo with the similar name but rather a module meant to plug with some other bigger projects.

Implementation details

• rename the crate name to pallet-chainbridge
• rename the repo to pallet-chainbridge

•   construct_runtime!(
             ...
    	Chainbridge: pallet_chainbridge,
    );
  

Testing details

Acceptance Criteria

If there are not enough votes for a proposal to pass, the proposal will hang in the system forever and can be voted and executed later. The problem is that it’s impossible to cancel this proposal in order to send a new one and be sure that the old one will never be executed.

We've restricted the maximum balance size to 128bits here:

We should be able to modify this to allow for a conversion from whatever size the BalanceOf is to a U256. Requires further investigation.

• Rename Validators to Relayers (should happen accross the board)
• Require root to whitelist chain, change validators and change EmitterAddress
• Add ability for root to change threshold
• EmitterAddress should be updated to better suit the notion of a chain ID (add to genesis)
• Remove need to store AccountId of voters by indexing them instead (see #8)
• Compute proposal hash on-chain -- maybe concat 2 separate hashes to make this easier
• Remove usages of unwrap
• Prune state; remove proposal after completion (part of block prod?) (see #8)
• Update vote against condition: votes against > validator set size - threshold
  • Add validator count
  • Update condition

Tests

• Test genesis config - https://github.com/centrifuge/substrate-pallet-multi-account/blob/a80ed7a071a449681ef57c95fa13b06b302a66f2/src/lib.rs#L849-L864
• Test events firing (see #7)

On the substrate side, when votes are happening, the following function is called to define whether the vote is completed or not:

chainbridge/src/lib.rs:L60-L70

fn try_to_complete(&mut self, threshold: u32, total: u32) -> bool {
    if self.votes_for.len() >= threshold as usize {
        self.status = ProposalStatus::Approved;
        true
    } else if self.votes_against.len() > (total - threshold) as usize {
        self.status = ProposalStatus::Rejected;
        true
    } else {
        false
    }
}

The problem is that after that call, the result of the vote is not checked. Only the fact that it has finished is taken into account.

While in most cases it’s true that the result of the completed vote can be predicted, there are some edge-cases when it’s not true. For example, when total or threshold variables are changed. That may lead to executing the proposal that should be canceled and vise versa.

Once we have specified how to handle this we need to update cancel_execution() which currently just emits an event.

We need to specify appropriate fees for calls.

For execution of proposals we can use something like this: https://github.com/centrifuge/substrate-pallet-multi-account/blob/master/src/lib.rs#L489-L492

Presently there is no event checks in the tests. These should be added.

Some useful code here: https://github.com/centrifuge/substrate-pallet-multi-account/blob/3a40d9a1107816ee28989bf4bbbe3f0ab6a1822d/src/lib.rs#L833-L842

The issue I was facing was when multiple events fire in a single call, only the last is returned by expect_event. We should extend the functionality to support n most recent events.

Events can be collected like this:

let events: Vec<Event> = system::Module::<Test>::events().iter().map(|e| e.event.clone()).collect();

To improve readability and usability migrate to newer substrate frame macros

There is already significant progress here #94

Currently everything is contained in one source file, to give the pallet a bit more structure we can split off types etc..

Some reference code: https://github.com/parami-protocol/parami-blockchain/tree/main/pallets/chainbridge

This epic is meant to house all issues related to the update/upgrade of the ChainBridge pallet. This epic is not limited to a certain number of tasks, as more tasks may be added in as they are discovered/determined to be relevant.

Presently we store the account IDs of all validators who have voted for all proposal. This is inefficient in two ways:

1. The account ids are stored for every proposal,
2. Over time this may grow large, past proposals will eventually become irrelevant.

To address these we should:

1. Use indexing to identify relayers. Rather than store the accountIds, we can simply assign indices to the relayers, and at some checkpoint allow relayers to be added/removed the and indices updated accordingly.

2. Consider some pruning method to remove completed proposals. Perhaps we can remove a proposal immediately after completion?

There's a lot of share code between the two pallets for mocking/testing. We should move this into a shared crate.

More examples will help explain what's possible with chainbridge. We should include more examples such as token transfer out of the chain, and executing a call that requires signed origin.

With this, we should have relayers check the existence of the call on chain prior to calling acknowledge_proposal and provide only the hash of the call if it already exists on chain.

Similarly, for rejecting a proposal we do not need to require the call be provided at all.

We should use T::Balance for amounts on fungible transfers.

In Substrate it’s only possible to calculate voting results only when a new vote is submitted. But if total or threshold changes, it should be possible to check the results of the current votings and call finalize_execution(or cancel_execution) without submitting a vote.

Almost the same problem we have in solidity. Even though it’s possible to execute the proposal without making a vote, in order to do that, status of the proposal should be ProposalStatus.Passed which can only be assigned when making a vote.

Recommendation
Add public function that checks if a vote can be completed and executed.

It doesn't make much sense for the chainId to change. It should be immutable from genesis.

Use updated benchmark macros

Hello,
I am getting this error while building this repo:-

error[E0283]: type annotations needed
  --> example-pallet/src/lib.rs:80:85
   |
80 |             <bridge::Module<T>>::transfer_fungible(dest_id, resource_id, recipient, U256::from(amount.saturated_into()))
   |                                                                                     ^^^^^^^^^^ cannot infer type for type parameter `T` declared on the trait `From`
   |
   = note: cannot satisfy `U256: From<_>`
   = note: required by `std::convert::From::from`

I think if we add with saturated into then it will work something like this:-
<bridge::Module<T>>::transfer_fungible(dest_id, resource_id, recipient, U256::from(amount.saturated_into::<u128>()))

We should use our custom types, and include the source ID for proposal related events.

• Update amount type
• Remove recipient from hash transfer
• remove chain id from deposit event (?)