Reworked pallets for XCMv3 #791
Conversation
vgeddes
changed the base branch from
main
to
vincent/remove-snowbridge-parachain
|
|
||
| /// Weight information for extrinsics in this pallet | ||
| type WeightInfo: WeightInfo; | ||
|
|
||
| type Reward: Get<BalanceOf<Self>>; |
There was a problem hiding this comment.
Non blocking: Parity's bridge implementation for rococo-wococo. The fee is configured more dynamically. It still requires governance to change, but it is decoupled from the runtime. It might be useful for us to decouple updating fees from runtime updates as well.
There was a problem hiding this comment.
Nice catch, added ticket in https://linear.app/snowfork/issue/SNO-402
| @@ -9,7 +9,7 @@ echo 'Running pre-commit hook...' | |||
| chronic typos . | |||
|
|
|||
| # lint and format for core contracts and typescript codes | |||
| (cd core && chronic pnpm lint && chronic pnpm format) | |||
| (cd core && chronic pnpm lint) | |||
There was a problem hiding this comment.
Anything break so we remove pnpm format here? AFAIK it will just run the underlying forge fmt so should be fine?
There was a problem hiding this comment.
I found it a bit confusing for a pre-commit hook to alter the code automatically. I disabled it for that reason.
I would rather we enable "Format on Save" option in VS code. For Solidity:
{
"[solidity]": {
"editor.formatOnSave": true,
"editor.defaultFormatter": "JuanBlanco.solidity"
},
"solidity.formatter": "forge",
}
Then at least we know what changes we are committing when we run "git commit"
| "cumulus/**" | ||
| "cumulus/**", | ||
| "smoketest/src/parachains", | ||
| "smoketest/src/contracts", |
There was a problem hiding this comment.
Do we have these smoketest scripts ready? can't find in this PR.
There was a problem hiding this comment.
I experimented with writing smoketest scripts in Rust, but deleted the experiment as they were too slow to compile, and Rust's test frameworks are not as good as Golang's.
But see this in case we ever want to generate Rust clients for our pallets and contracts: https://www.notion.so/snowfork/Rust-bindings-b6566f13dd894ac1ac9a347f9bbbf160?pvs=4
| <LatestVerifiedBlockNumber<T>>::set(block_number); | ||
| // Reward relayer from the sovereign account of the destination parachain | ||
| let dest_account = envelope.dest.into_account_truncating(); | ||
| T::Token::transfer(&who, &dest_account, T::Reward::get(), Preservation::Expendable)?; |
There was a problem hiding this comment.
Seems a little confused. Is the transfer logic here correct or just some temp placeholder? Is impl supposed to be some xcm call sent to asset chain?
There was a problem hiding this comment.
And also seems we remove the previous logic <LatestVerifiedBlockNumber<T>>::set(block_number); Is it safe to remove? I would assume we still need some scanner logic to find the unsynced messages and IMO probably useful to narrow the scope of scan down with the checkpoint.
There was a problem hiding this comment.
Seems a little confused. Is the transfer logic here correct or just some temp placeholder? Is impl supposed to be some xcm call sent to asset chain?
That's just for rewarding the relayer. We definitely still need to implement the routing of the XCM call.
There was a problem hiding this comment.
I temporarily removed the LatestVerifiedBlockNumber logic because the checkpoint logic in the relayer didn't work with multiple lanes (i.e each parachain has its lane and nonce).
So I am may add it back once I rewrite the relayer properly.
| } | ||
|
|
||
| err = messages.Sync(ctx, eg) | ||
| sub, err := contract.WatchMessage(&opts, messages, [][]byte{key}) |
There was a problem hiding this comment.
So to confirm, WatchMessage here will only subscribe to the latest message events and we still need some scanner logic to find unsynced messages? Also some related comments here
There was a problem hiding this comment.
yeah, this is just a very simple, incomplete implementation that allows us to test the on-chain code. We will need to rewrite it in a future PR.
There was a problem hiding this comment.
Also we may have to modify the execution message relayer to support the optimizations we talked about on slack (execution headers, etc).
There was a problem hiding this comment.
And not sure even if WatchMessage will work here because we can not verify the execution block if it is not finalized in beacon chain yet. so maybe the previous FilterMessage with FilterOpts(i.e. a bounded range) make more sense.
The start of the range should be the LatestVerifiedBlockNumber(though revamp required for multiple lanes support) in message layer and the end is the LatestExecutionHeaderState(also need to optimize as discussed) in beacon layer?
There was a problem hiding this comment.
I check here that the execution header has already been imported: https://github.com/Snowfork/snowbridge/pull/791/files#diff-ba222152cf61a695305ad44e12ccc757ffa95a1f6c1c8ee698e21a7650155229R102
But yes, when we re-implement the relayer properly, we will likely use FilterMessage along with the following algorithm:
- Check message nonce on Parachain side
- Check message nonce on Ethereum side
- Compare nonces to determine which messages need to be synced.
- Scan Ethereum blocks using
FilterMessageto find theOutboundQueue.Messageevent logs. This is fast, as Ethereum indexes events, so we can almost lookup event logs by nonce:
Then as you say, we will use LatestExecutionHeaderState to check that execution header has been imported first.
Changes:
TestTokenreplaced with WETH9 (Ether wrapped in an ERC20 token).I also disabled the parachain message relayer. It still needs to be updated to be compatible with these refactorings over the past month.
The ethereum execution message relayer is operational, but still needs more work to be production ready.
Fixes: SNO-396, SNO-402