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

Migrate domain v1 primitives to v2 #1684

Merged
merged 14 commits into from
Jul 24, 2023
Merged

Migrate domain v1 primitives to v2 #1684

merged 14 commits into from
Jul 24, 2023

Conversation

NingLin-P
Copy link
Member

@NingLin-P NingLin-P commented Jul 21, 2023

close #1676

This migrate domain v1 primitives to v2, including changes of:

  • Construct the domain v2 Bundle and ExecutionReceipt in the domain client
  • Bring back the domain runtime api like head_receipt_number, oldest_receipt_number
  • Remove all the v1 usage
  • Enforce the domain config's domain block limit when constructing bundle
  • Revise the extract_signer runtime API to fix an issue that may filter out all the extrinsic when building domain block

Code contributor checklist:

This commit include changes of:
- remove BundleHeader::operator_id as it is duplicated with ProofOfElection::operator_id
- add ExecutionReceipt::domain_block_hash which is needed in the fruad proof
- replace Bundle::execution_trace to ExecutionReceipt::execution_trace to store trace on chain
- add some misc helper functions

Signed-off-by: linning <linningde25@gmail.com>
It will be used later to construct the v2 ExecutionReceipt

Signed-off-by: linning <linningde25@gmail.com>
…perator

Signed-off-by: linning <linningde25@gmail.com>
Signed-off-by: linning <linningde25@gmail.com>
Signed-off-by: linning <linningde25@gmail.com>
Signed-off-by: linning <linningde25@gmail.com>
Calling Checkable::check before initialize_block will result in AncientBirthBlock
error due to some check in CheckMortality::additional_signed, and cause all the
extrinsics being filtered out

Signed-off-by: linning <linningde25@gmail.com>
…n test

Signed-off-by: linning <linningde25@gmail.com>
This test along with test_domain_tx_propagate is disabled due to they are using
Call::transfer internally but the evm genesis config is empty and there is no
initial balance in the testing account thus these test will failed, leavea a
TODO for now will fix in the future.

Signed-off-by: linning <linningde25@gmail.com>
Signed-off-by: linning <linningde25@gmail.com>
Signed-off-by: linning <linningde25@gmail.com>
The __construct_runtime_integrity_test::runtime_integrity_tests test enforce the pallet
call enum size should be smaller than 1024 bytes, the submit_fraud_proof call break this
limit because the v2 domain header contains a receipt and the bundle equivocation proof
contains 2 headers.

Signed-off-by: linning <linningde25@gmail.com>
Copy link
Contributor

@liuchengxu liuchengxu left a comment

Choose a reason for hiding this comment

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

A few minor comments. I believe the next step is to complete the bundle verification in terms of the new fields in v2.

domains/client/domain-operator/src/bundle_processor.rs Outdated Show resolved Hide resolved
crates/pallet-domains/src/tests.rs Show resolved Hide resolved
crates/sp-domains/src/lib.rs Show resolved Hide resolved
Copy link
Contributor

@vedhavyas vedhavyas left a comment

Choose a reason for hiding this comment

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

Make sense in general. Left some nits. Will look again in sometime

One thing I have noticed is that bunch of these smaller commits would actually make sense when they are merged together. Commits that just adds tests could be part of the commit that introduced the change. I recommend re-reading our contribution guidelines

crates/sp-domains/src/v2.rs Outdated Show resolved Hide resolved
@NingLin-P
Copy link
Member Author

One thing I have noticed is that bunch of these smaller commits would actually make sense when they are merged together. Commits that just adds tests could be part of the commit that introduced the change. I recommend re-reading our contribution guidelines

Commit message is similar but the change they bring is not, the test is an integration test and is not for any single specified commit.

@NingLin-P NingLin-P merged commit 561f0e5 into main Jul 24, 2023
9 checks passed
@NingLin-P NingLin-P deleted the domain-client-v2 branch July 24, 2023 14:44
@NingLin-P NingLin-P mentioned this pull request Jul 27, 2023
30 tasks
ParthDesai added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Aug 4, 2023
ParthDesai added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Aug 15, 2023
ParthDesai added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Aug 15, 2023
ParthDesai added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Aug 15, 2023
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.

Migrate v1 primitives to v2 in the domain client
3 participants