-
Notifications
You must be signed in to change notification settings - Fork 46
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
implement per-parentchain extrinsic factory and metadata #1501
base: master
Are you sure you want to change the base?
Conversation
where | ||
ParentchainApi: AccountApi<AccountId = AccountId, Balance = Balance, Index = Index> | ||
+ GetBalance<Balance = Balance> | ||
+ GetTransactionPayment<Balance = Balance> | ||
+ BalancesExtrinsics< | ||
Address = Address, | ||
Balance = Balance, | ||
Extrinsic<TransferAllowDeathCall<Address, Balance>> = UncheckedExtrinsicV4< | ||
Address, | ||
TransferAllowDeathCall<Address, Balance>, | ||
<ParentchainApi as ChainApi>::Signature, | ||
<ParentchainApi as ChainApi>::SignedExtra, | ||
>, | ||
> + SubmitAndWatch<Hash = Hash> | ||
+ ChainApi<Signer = sr25519::Pair>, | ||
ParentchainApi::Signature: Encode, | ||
ParentchainApi::SignedExtra: Encode, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is so horribly verbose. there has to be another way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can create an aggregate trait like here: https://stackoverflow.com/a/26983395/19331449
(parentchain_handler, last_synced_header) | ||
} | ||
|
||
fn init_integritee_parentchain<E>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was just an attempt to solve the mess with redundant but explict code. not sure that's the way
} | ||
} | ||
|
||
fn send_extrinsic( | ||
fn send_integritee_extrinsic( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to be generic over parentchains. we only need this for Integritee Network
pub type TargetBRuntimeConfig = | ||
WithExtrinsicParams<AssetRuntimeConfig, target_b::ParentchainExtrinsicParams>; | ||
|
||
pub enum ParentchainApiLocal { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this I somehow like and it seems to work for OCall. Maybe we could make it work for service-main instead of the ParentchainApiWrapper
shizzle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this is the way to go actually for many things, also when we distinguish build flavors.
// fixme: this is an ugly hack because api-client's 'new()' isn't part of any trait we could implement therefore we can't use the fn new() on generic api types | ||
impl ParentchainApiWrapper for IntegriteeParentchainApiWrapper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really ugly. there has to be a better way. but we may need to patch api-client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrapper not necessary, we can just define our own trait and implement it on the api client IMO, which is a bit better.
@@ -103,7 +106,7 @@ impl TargetAParachainHandler { | |||
extrinsics_factory.clone(), | |||
)?, | |||
WorkerMode::Sidechain => | |||
unimplemented!("Can't run target a chain in sidechain mode yet."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything I fail to see why this will not work?
@clangenb I think I need an intermediate review as I seem to get something entirely wrong. status:
reproduce my errors in root folder build: |
@@ -40,8 +40,10 @@ log = { version = "0.4", default-features = false } | |||
# substrate dep | |||
binary-merkle-tree = { default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v0.9.42" } | |||
frame-support = { default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v0.9.42" } | |||
frame-system = { default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v0.9.42" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std enablement
@@ -20,6 +20,7 @@ | |||
#[cfg(all(feature = "std", feature = "sgx"))] | |||
compile_error!("feature \"std\" and feature \"sgx\" cannot be enabled at the same time"); | |||
|
|||
extern crate core; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
// fixme: this is an ugly hack because api-client's 'new()' isn't part of any trait we could implement therefore we can't use the fn new() on generic api types | ||
impl ParentchainApiWrapper for IntegriteeParentchainApiWrapper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrapper not necessary, we can just define our own trait and implement it on the api client IMO, which is a bit better.
pub type TargetBRuntimeConfig = | ||
WithExtrinsicParams<AssetRuntimeConfig, target_b::ParentchainExtrinsicParams>; | ||
|
||
pub enum ParentchainApiLocal { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this is the way to go actually for many things, also when we distinguish build flavors.
// #TODO: #1451: Reintroduce `ParentchainApi: ChainApi` once there is no trait bound conflict | ||
// any more with the api-clients own trait definitions. | ||
impl<EnclaveApi> ParentchainHandler<ParentchainApi, EnclaveApi> | ||
impl<EnclaveApi> ParentchainHandler<IntegriteeParentchainApi, EnclaveApi> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are exactly right. I believe this is the root cause of the problem. The api-client has re-defined types and traits because they were not available in no-std in substrate < v0.9.43. When I updated the api-client to its newest version, but v0.9.42, I had to reintroduce these types again: scs/substrate-api-client@ea49a0d.
Then I was essentially in the same rabbit whole as you: certain trait-bounds weren't satisfied because we are using the actual types from substrate whereas the api-client uses its types/traits in its traitbounds.
My recommedation is: if you have to use generics in those api taits, we unfortunately have to upgrade the polkadot version and the api-client.
will pause work here, hoping for teaclave to bump rustc so we can upgrade to Polkadot 1.3.0 and api-client 0.15.0 |
needed to support Asset Hub (or Encointer) as Target A parentchain
major steps: