-
Notifications
You must be signed in to change notification settings - Fork 115
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
Argo
bridge runtime pallet
#5134
Comments
@kdembler maybe you mean BridgeConfigUpdated? |
@kdembler on |
@freakstatic the goal is to make sure the user is aware of the current fee. Without it this could happen:
With |
Oh yeah that makes sense, should it be a exact match or should we defined an acceptable interval? |
Another question, what should be the value of the |
Should be exact, I don't expect this to happen often.
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. |
Notes from the meeting on 23/04/2024:
|
@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 // 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. |
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. |
Following EIP-155 Eth mainnet is @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 |
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.
Issue is runtime does not need to read it. |
@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? |
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. |
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. |
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. |
The upsides:
The only downside I see is slightly lower flexibility if we need additional data in the future. In my opinion the upsides win |
See background in #5084
Goal
Develop
argoBridge
runtime pallet that will be used for Argo bridge operations.Required functionalities:
Pallet
State
status
: enum with options{ type: 'active' } | { type: 'paused' } | { type: 'thawn'; thawn_started_at: 123456 }
.operator_account
: account ID.pauser_accounts
: list of account IDs with permission to pause the bridge operations.mint_allowance
: number indicating number of tokens that the bridge pallet is able to mint. This should always equaltotal_burned - total_minted
.bridging_fee
: amount of JOY burned as a fee for each transfer.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:
operator_account
pauser_accounts
bridging_fee
thawn_duration
Emits
BridgeConfigUpdated(config)
eventExtrinsics
request_outbound_transfer(dest_account, amount, expected_fee)
- callable by anybody:status
is{ type: 'active' }
.amount
of JOY from sender account, increasesmint_allowance
byamount
transfer_id
by hashing(dest_account, amount, block_number, call_index_in_block)
OutboundTransferRequested(transfer_id, dest_account, amount, paid_fee)
finalize_inbound_transfer(dest_account, amount, request_block, request_nonce)
- callable byoperator_account
only:status
is{ type: 'active' }
.transfer_id
by hashing(dest_account, amount, request_block, request_nonce)
amount
is not bigger than currentmint_allowance
mint_allowance
byamount
, mintsamount
of JOY tokens todest_account
InboundTransferFinalized(transfer_id)
pause_bridge()
- callable by any ofpauser_accounts
:status
to{ type: 'paused' }
BridgePaused(pauser)
init_unpause_bridge()
- callable by any ofpauser_accounts
:status
to{ type: 'thawn'; thawn_started_at: current_block }
BridgeThawnStarted(pauser)
finish_unpause_bridge()
- callable byoperator_account
only:status.type === 'thawn
&& current_block > status.thawn_started_at + thawn_duration`status
to{ type: 'active' }
BridgeThawnFinished()
The text was updated successfully, but these errors were encountered: