Skip to content
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

Upgrade substrate to 7406442bea0194ffcafc4e8d48d895d4d8d11346 #540

Merged
merged 2 commits into from
Dec 6, 2021

Conversation

echevrier
Copy link
Contributor

@echevrier echevrier commented Nov 30, 2021

For compatibility reason, the node requiring V14 metadata integritee-network/integritee-node#83

@echevrier echevrier linked an issue Nov 30, 2021 that may be closed by this pull request
@echevrier
Copy link
Contributor Author

echevrier commented Dec 2, 2021

Please don't merge for now.
I've tested it locally and it seems to work with node branch 83/upgrade_substrate_dep_v14 and sgx_runtime branch ec_upgrade_substrate_7406442bea0194ffcafc4e8d48d895d4d8d11346
It also needs theses branched to compile.

Copy link
Contributor

@clangenb clangenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, only one minor comment, otherwise we can merge. Thanks for your hard work on this one!

loop {
let ret: ProcessedParentchainBlockArgs = _chain_api
.wait_for_event::<ProcessedParentchainBlockArgs>(
TEEREX,
"ProcessedParentchainBlock",
Some(decoder.clone()),
None,
Copy link
Contributor

@clangenb clangenb Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, this is obsolete in the api-client. I need to remove that arg.

pub fn grandpa_log<Block: BlockT>(
digest: &DigestG<HashFor<Block>>,
) -> Option<ConsensusLog<NumberFor<Block>>> {
pub fn grandpa_log<Block: BlockT>(digest: &DigestG) -> Option<ConsensusLog<NumberFor<Block>>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename the import to just Digest. The G stands for generic, which is not the case anymore.

@clangenb clangenb self-requested a review December 2, 2021 13:40
Copy link
Contributor

@haerdib haerdib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Few minor comments.

let id = OpaqueDigestItemId::Consensus(&GRANDPA_ENGINE_ID);
digest.convert_first(|l| l.try_to::<ConsensusLog<NumberFor<Block>>>(id))
}

pub fn pending_change<Block: BlockT>(
digest: &DigestG<HashFor<Block>>,
digest: &DigestG,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as clangenbs comment above: Remove G

enclave-runtime/Cargo.toml Show resolved Hide resolved
enclave-runtime/Cargo.toml Show resolved Hide resolved
echevrier added 2 commits December 6, 2021 11:34
Compile with error  duplicate lang item in crate `sgx_tstd`

Sgx runtime upgraded to new substrate, but has extrinsics-factory errors.

Upgrade works, problem in substrate-api-client fixed in version #a4d931c85f30ba22ff1f982c47caf57fe43ebc90

Rebase to master and update changes to new substrate

cargo clippy

Remove patch to local dependencies

Remove comments

Update spec version to 4

Changes from review
@echevrier echevrier force-pushed the ec_upgrade_substrate_v14metadata branch from 4c2cbec to 820d72d Compare December 6, 2021 13:35
@echevrier echevrier requested a review from haerdib December 6, 2021 14:55
Copy link
Contributor

@haerdib haerdib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks :)

@haerdib haerdib merged commit 5149f05 into master Dec 6, 2021
@echevrier echevrier deleted the ec_upgrade_substrate_v14metadata branch December 6, 2021 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

upgrade substrate dep
3 participants