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

feat: new PublishTemplate instruction #1208

Conversation

ksrichard
Copy link
Collaborator

Description

As part of #1207 , this PR contains the basic instruction and skeleton for instruction processing. There is a validation implemented for the new PublishTemplate instruction (where also we validate size of wasm binary).

Please note that in this PR there is no new substates or anything generated yet.

Motivation and Context

#1207

How Has This Been Tested?

Send a new transaction to VN:

    let AccountGetResponse { account, public_key, } = client
        .accounts_get(ComponentAddressOrName::Name("acc"))
        .await?;
    let account_component_address = account.address
        .as_component_address()
        .expect("Failed to get component address");

    // publish wasm template
    let wasm_binary = fs::read("./templates/counter/target/wasm32-unknown-unknown/release/counter.wasm")?;
    let tx = Transaction::builder()
        .fee_transaction_pay_from_component(account_component_address, Amount(1000))
        .publish_template(wasm_binary)
        .build_unsigned_transaction();
    let transaction_submit_req = TransactionSubmitRequest {
        transaction: tx,
        signing_key_index: Some(account.key_index),
        detect_inputs: true,
        detect_inputs_use_unversioned: true,
        proof_ids: vec![],
        autofill_inputs: vec![],
    };
    let resp = client.submit_transaction(transaction_submit_req).await?;
    println!("Submit RESP: {resp:?}");
    let wait_req = TransactionWaitResultRequest {
        transaction_id: resp.transaction_id,
        timeout_secs: Some(120),
    };
    let wait_resp = client.wait_transaction_result(wait_req).await?;
    println!("TX RESP: {wait_resp:?}");

Check printed out result and transaction result on UIs.

Examples:

  • Invalid wasm template binary
    image

  • Size exceeding wasm template (manually set a lower limit to check error)
    image

  • Success
    image

What process can a PR reviewer use to test or verify this change?

Breaking Changes

  • None
  • Requires data directory to be deleted
  • Other - Please specify

Copy link

github-actions bot commented Dec 2, 2024

Test Results (CI)

561 tests   - 21   561 ✅  - 21   1h 22m 46s ⏱️ - 1h 35m 28s
 54 suites  - 11     0 💤 ± 0 
  1 files    -  1     0 ❌ ± 0 

Results for commit b3ace13. ± Comparison against base commit 4d1117b.

This pull request removes 21 tests.
Scenario: Claim and transfer confidential assets via wallet daemon: tests/features/wallet_daemon.feature:88:3
Scenario: Claim base layer burn funds with wallet daemon: tests/features/claim_burn.feature:9:3
Scenario: Concurrent calls to the Counter template: tests/features/concurrency.feature:7:3
Scenario: Confidential transfer to unexisting account: tests/features/transfer.feature:160:3
Scenario: Counter template registration and invocation multiple times: tests/features/counter.feature:48:3
Scenario: Counter template registration and invocation once: tests/features/counter.feature:8:3
Scenario: Create account and transfer faucets via wallet daemon: tests/features/wallet_daemon.feature:8:3
Scenario: Create and mint account NFT: tests/features/wallet_daemon.feature:130:3
Scenario: Create resource and mint in one transaction: tests/features/nft.feature:81:3
Scenario: Double Claim base layer burn funds with wallet daemon. should fail: tests/features/claim_burn.feature:48:3
…

♻️ This comment has been updated with latest results.

@@ -19,6 +19,7 @@ tari_template_builtin = { workspace = true }
tari_template_lib = { workspace = true }
tari_utilities = { workspace = true }
tari_transaction = { workspace = true }
tari_consensus = { workspace = true }
Copy link
Member

@sdbondi sdbondi Dec 4, 2024

Choose a reason for hiding this comment

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

Consensus and execution must remain decoupled.

I see you're using consensus constants to dictate the maximum permitted size of a template.

I suggest creating a TransactionProcessorConfig struct that you pass into the processor that we can configure using consensus constants in the validator/indexer's bootstrap.rs. The network can also go into that config as well as any future config.

pub struct TransactionProcessorConfig {
   pub network: Network,
  pub template_binary_max_size_bytes: usize
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay, totally makes sense, let me update

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just pushed

Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

LGTM - you need to regenerate typescript bindings

@ksrichard
Copy link
Collaborator Author

@sdbondi I just pushed changes, now it should be okay

sdbondi
sdbondi previously approved these changes Dec 5, 2024
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

utACK

dan_layer/template_test_tooling/src/template_test.rs Outdated Show resolved Hide resolved
@sdbondi sdbondi disabled auto-merge December 6, 2024 04:43
@sdbondi sdbondi merged commit 50fbf05 into tari-project:development Dec 6, 2024
11 of 12 checks passed
sdbondi added a commit to sdbondi/tari-dan that referenced this pull request Dec 6, 2024
* development:
  feat: new PublishTemplate instruction (tari-project#1208)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants