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

Shielding parentchain transfers #1378

Merged
merged 50 commits into from
Nov 15, 2023
Merged

Conversation

coax1d
Copy link
Contributor

@coax1d coax1d commented Jun 22, 2023

closes #1252
Built ontop of #1355

Adding in initial shielding using Alices account on Parentchain.

Involves refactoring:

  • introduced ita_parentchain_interface crate where all listeners are moved to now (extrinsic and event triggering).
  • this crate does discriminate ParentchainId for future generalization

testing

run node on host:

./target/release/integritee-node --dev --tmp --unsafe-ws-external --rpc-cors all 

run sidechain

SGX_MODE=SW WORKER_MODE=sidechain WORKER_FEATURES=dcap make
cd bin
export RUST_LOG=info,substrate_api_client=warn,ws=warn,mio=warn,its_consensus_common=info,sidechain=info,integritee_service=trace,enclave_runtime=trace,ac_node_api=warn,sp_io=warn,itc_parentchain_indirect_calls_executor=trace,itp_stf_executor=trace,itc_parentchain_light_client=trace,itc_parentchain_block_importer=trace,itp_stf_state_handler=trace,ita_stf=trace,itp-attestation-handler=debug,itc_parentchain_indirect_calls_executor=trace,itp_top_pool=debug,itc_offchain_worker_executor=debug,ita_parentchain_interface=trace
./integritee-service -c -u ws://172.17.0.1 run --skip-ra --dev &> worker.log 

once producing sidechain blocks, use js/apps to send funds from Bob to Alice

  • worker log should show shielding for ....
  • and ita_stf::trusted_cal[....] balance_shield([...]

check shielded balance for Bob

./integritee-cli -u ws://172.17.0.1 trusted --mrenclave 2MyXuroF4yiR8A6S8t334C4ZvY6qyKxVBKsd8SPnVW3y balance //Bob

should return the amount you sent
q.e.d.

@coax1d coax1d added A0-core Affects a core part A2-applibs Affects app-libs, i.e. runtime or stf A3-sidechain B1-releasenotes labels Jun 22, 2023
@coax1d coax1d requested a review from clangenb June 22, 2023 14:23
@coax1d coax1d self-assigned this Jun 22, 2023
coax1d and others added 4 commits June 22, 2023 16:23
# Conflicts:
#	Cargo.lock
#	core/parentchain/indirect-calls-executor/src/event_filter.rs
#	core/parentchain/indirect-calls-executor/src/executor.rs
#	enclave-runtime/Cargo.lock
#	enclave-runtime/src/test/top_pool_tests.rs
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.

I think this goes into the right direction, but the executor mustn't be aware of the business logic specifics. :)

core/parentchain/indirect-calls-executor/src/executor.rs Outdated Show resolved Hide resolved
core/parentchain/indirect-calls-executor/src/executor.rs Outdated Show resolved Hide resolved
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.

Sorry for the very late review. :)

Nice, this looks quite good now! According to your judgement, what is missing in this PR?

I will merge master into this branch, as the breaking changes with the api-client might confuse you a bit.

core/parentchain/indirect-calls-executor/src/executor.rs Outdated Show resolved Hide resolved
app-libs/stf/src/lib.rs Outdated Show resolved Hide resolved
# Conflicts:
#	Cargo.lock
#	core-primitives/types/src/parentchain.rs
#	core/parentchain/indirect-calls-executor/src/filter_metadata.rs
#	enclave-runtime/Cargo.lock
@clangenb clangenb added C1-low 📌 Does not elevate a release containing this beyond "low priority" E5-publicapi PR breaks public API labels Oct 3, 2023
@coax1d coax1d marked this pull request as ready for review November 3, 2023 12:09
@brenzi
Copy link
Collaborator

brenzi commented Nov 4, 2023

CI M6 fails because choosing //Alice as the vault account disturbs other tests that use //Alice for other purposes. concretely, demo_shielding_unshielding fails because unshielding to //Alice triggers the new shielding logic (which panics, which it shouldn't, but that's for further investigation)

@brenzi
Copy link
Collaborator

brenzi commented Nov 4, 2023

these cyclic dependencies are a PITA.

@brenzi brenzi requested a review from clangenb November 15, 2023 09:33
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.

All in all, locks good. With some minor modifications to trait definitions, I think we can make the code more ergonomic in the codebase. As associated types don't have to be mentioned usually, when a trait is put as a trait bound.

@brenzi brenzi merged commit 4689acf into master Nov 15, 2023
37 checks passed
@clangenb clangenb deleted the coax1d-shielding-parentchain-transfers branch November 15, 2023 23:47
@clangenb clangenb restored the coax1d-shielding-parentchain-transfers branch November 15, 2023 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-core Affects a core part A2-applibs Affects app-libs, i.e. runtime or stf A3-sidechain B1-releasenotes C1-low 📌 Does not elevate a release containing this beyond "low priority" E5-publicapi PR breaks public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

subscribe to balances.transfer events and shield funds
3 participants