-
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
Polkadot v0.9.37 #1165
Polkadot v0.9.37 #1165
Conversation
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.
Nice, thanks for the hard work of updating the api-client! I have one systematic change request here, which should improve the overall code because it removes the need for defining the Runtime
in most cases.
When you apply my suggestion, be aware that you can also remove the my-node-runtime
dependency in some cases.
core-primitives/node-api/api-client-extensions/src/pallet_teerex_api_mock.rs
Outdated
Show resolved
Hide resolved
Hi @clangenb , yes I am expecting comments like this will come. About changes related to api trait, I had discussion with @haerdib and did some changes with her together. I don't have a chance to check carefully what's the difference of substrate-api-client between branch polkadot-v0.9.37 and master. Actually there are quite some changes. If I understand right, the issue scs/substrate-api-client#406 you mentioned, should be implemented/refactored before we apply the api trait changes. What a pity I didn't see it before. So here I can see 3 options:
@clangenb @haerdib @Kailai-Wang What's your opinion? |
Sorry if I was not being clear. Yes, scs/substrate-api-client#406 will help to simplify the trait bounds here. However, my suggested change is unrelated to that. I only recommend removing the generic trait parameter from Before: pub trait ChainApi<Runtime> {
// some traitbounds...
fn get_genesis_hash(&self) -> ApiResult<Runtime::Hash>
} after // This is actually the same pattern that the api-client already uses: https://github.com/scs/substrate-api-client/blob/07ed77d01c0d0205305e6bbff32525b53a5fbd05/src/extrinsic/balances.rs#L61
pub trait ChainApi {
// no traitbounds needed in the trait declaration
type Hash;
fn get_genesis_hash(&self) -> ApiResult<Self::Hash>
}
impl<P: Pair, Client: RpcClient, Params, Runtime> ChainApi
for Api<P, Client, Params, Runtime>
where
// same traitbounds as with the other approach.
MultiSignature: From<P::Signature>,
Params: ExtrinsicParams<Runtime::Index, Runtime::Hash>,
Runtime: BalancesConfig,
Runtime::Hash: FromHexString + From<H256> + Into<H256>,
Runtime::Index: Into<u32> + Decode,
Runtime::Balance: TryFrom<NumberOrHex> + FromStr + Into<u128>,
{
type Hash = Runtime::Hash
} This in turn leads to the following difference: before use my_node_runtime::Runtime;
pub(crate) fn sync_state<
E: TlsRemoteAttestation + EnclaveBase,
NodeApi: PalletTeerexApi<Runtime>,
WorkerModeProvider: ProvideWorkerMode,
> after // no runtime import needed.
// use my_node_runtime::Runtime;
pub(crate) fn sync_state<
E: TlsRemoteAttestation + EnclaveBase,
NodeApi: PalletTeerexApi,
WorkerModeProvider: ProvideWorkerMode,
> |
Btw. sorry if you thought that I was dissatisfied. I think the PR is great except for my little change request. :) |
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.
Thank you so much for the fixes, this looks exactly how I imagined it! Some minor comments remain and then we are good to go!
core-primitives/node-api/api-client-extensions/src/pallet_teerex_api_mock.rs
Outdated
Show resolved
Hide resolved
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.
Great, thanks a lot for the fixes! Will merge as soon as the CI passes.
There seems to be a startup error in all the demos:
This is when it is trying to get the nonce from the chain. |
Closing in favor of this one: #1173 I am sorry for the extra effort you invested. |
Don't worry. Let's looking forward to the next upgrade. |
This is an update according to substrate (version polkadot-v0.9.37).
Big part of the update is still version bump up in toml files.
But @clangenb I am expecting some comment may come from you about the substrate-api-client part (mainly in commit: ab24686). I am crossing my fingers.