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

Fix production feature propagation #2439

Merged

Conversation

Kailai-Wang
Copy link
Collaborator

Context

It's not a flawless PR due to time limit. I only make sure critical part is covered.

To solve it perfectly we need to add the feature propagation in each intermediate crate (including the node/runtime), and/or have a smarter way to deal with the conditional compilation.

@Kailai-Wang Kailai-Wang self-assigned this Jan 29, 2024
Copy link

linear bot commented Jan 29, 2024

@kziemianek
Copy link
Member

kziemianek commented Jan 29, 2024

Compilation doesn't fail if I put this

match assertion {
			// the signer will be checked inside A13, as we don't seem to have access to ocall_api here
			Assertion::A13(_) => (),
			_ => if_production_or!(
				compile_error!("This macro should fail"),
				ensure!(
					ensure_self(&signer, &who) || ensure_alice(&signer_account),
					"Unauthorized signer",
				)
			),
		}

in litentry-parachain/tee-worker/litentry/core/vc-issuance/lc-vc-task-receiver/src/lib.rs - which indicates that production feature was not applied.
Compilation fails if I add this compile_error macro invocation to getter.rs

Command:

make SGX_MODE=SW

Do you know why ?

@Kailai-Wang
Copy link
Collaborator Author

You didn't compile with SGX_PRODUCTION=1, so no production feature, thus no compile error - guess that's expected?

Copy link
Member

@kziemianek kziemianek left a comment

Choose a reason for hiding this comment

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

Looks ok, thanks.
Checked if it works using compile_error macro.

@kziemianek
Copy link
Member

You didn't compile with SGX_PRODUCTION=1, so no production feature, thus no compile error - guess that's expected?

Yes, I was experimenting with modified Makefile and added production feature to WORKER_FEATURES . I forgot about this compilation option.

@Kailai-Wang Kailai-Wang enabled auto-merge (squash) January 29, 2024 21:07
Copy link
Collaborator

@BillyWooo BillyWooo left a comment

Choose a reason for hiding this comment

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

👍

@Kailai-Wang Kailai-Wang merged commit 8ddda3c into dev Jan 29, 2024
32 checks passed
@Kailai-Wang Kailai-Wang deleted the p-455-worker-cli-can-get-non-authorized-idgraph-in-prod branch January 29, 2024 21:28
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.

None yet

3 participants