Skip to content

Start appending tx hashes to emitted wasm events#4454

Merged
mergify[bot] merged 8 commits intomainfrom
tiago/inner-tx-event-attr
Mar 18, 2025
Merged

Start appending tx hashes to emitted wasm events#4454
mergify[bot] merged 8 commits intomainfrom
tiago/inner-tx-event-attr

Conversation

@sug0
Copy link
Copy Markdown
Collaborator

@sug0 sug0 commented Mar 8, 2025

Describe your changes

Include the hash of the inner and wrapper transactions in emitted wasm events.

Checklist before merging

  • If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
    • This will require 2 reviewers to approve the changes
  • If this PR requires changes to the docs or specs, a corresponding PR is opened in the namada-docs repo
    • Relevant PR if applies:
  • If this PR affects services such as namada-indexer or namada-masp-indexer, a corresponding PR is opened in that repo
    • Relevant PR if applies:

@sug0 sug0 force-pushed the tiago/inner-tx-event-attr branch from eaa6758 to 41ff010 Compare March 10, 2025 10:16
@sug0 sug0 changed the title Tiago/inner tx event attr Start appending tx hashes to emitted wasm events Mar 10, 2025
@sug0 sug0 marked this pull request as ready for review March 10, 2025 10:17
@sug0 sug0 added the enhancement New feature or request label Mar 10, 2025
sug0 added a commit that referenced this pull request Mar 10, 2025
@sug0 sug0 requested review from grarco and tzemanovic March 10, 2025 10:20
@github-actions github-actions bot added the breaking:api public API breaking change label Mar 10, 2025
@sug0 sug0 added ledger and removed breaking:api public API breaking change labels Mar 10, 2025
sug0 added a commit that referenced this pull request Mar 11, 2025
@sug0 sug0 force-pushed the tiago/inner-tx-event-attr branch from 9402a1e to c9a9a9f Compare March 11, 2025 09:11
@github-actions github-actions bot added the breaking:api public API breaking change label Mar 11, 2025
/// Identifier of an inner transaction in a batch.
pub struct InnerTxId<'tx> {
/// Hash of the wrapper transaction, if any.
pub wrapper_hash: Option<Cow<'tx, Hash>>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A bit unlucky that this is an option, I guess it derives from the fact that DispatchArgs::Raw carries an optional wrapper hash?

.tree
.insert(&event_type, HashSet::new());
}
if let Some(inner_tx_id) = inner_tx_id {
Copy link
Copy Markdown
Collaborator

@grarco grarco Mar 12, 2025

Choose a reason for hiding this comment

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

Nice solution to avoid breaking changes in terms of gas. In the future, if this is not to much of a pain to achieve, I'd still like to account for the gas of these attributes, but for now this is perfectly fine

@tzemanovic
Copy link
Copy Markdown
Collaborator

I pushed an update for the failing test, lgtm

@tzemanovic
Copy link
Copy Markdown
Collaborator

the breaking:api label is correct:

Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.40.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field TxCtx.wrapper_hash in /home/runner/work/namada/namada/crates/vm/src/host_env.rs:122

--- failure function_parameter_count_changed: pub fn parameter count changed ---

Description:
A publicly-visible function now takes a different number of parameters.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.40.0/src/lints/function_parameter_count_changed.ron

Failed in:
  namada_vm::wasm::run::tx now takes 8 parameters instead of 7, in /home/runner/work/namada/namada/crates/vm/src/wasm/run.rs:139

--- failure method_parameter_count_changed: pub method parameter count changed ---

Description:
A publicly-visible method now takes a different number of parameters.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.40.0/src/lints/method_parameter_count_changed.ron

Failed in:
  namada_vm::host_env::TxVmEnv::new now takes 16 parameters instead of 15, in /home/runner/work/namada/namada/crates/vm/src/host_env.rs:162

(from https://github.com/anoma/namada/actions/runs/13784293744/job/38548636027)

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 14, 2025

Codecov Report

Attention: Patch coverage is 86.13861% with 14 lines in your changes missing coverage. Please review.

Project coverage is 74.51%. Comparing base (3d15673) to head (7e7cd29).
Report is 43 commits behind head on main.

Files with missing lines Patch % Lines
crates/events/src/extend.rs 46.15% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4454      +/-   ##
==========================================
+ Coverage   74.50%   74.51%   +0.01%     
==========================================
  Files         339      339              
  Lines      110638   110895     +257     
==========================================
+ Hits        82428    82631     +203     
- Misses      28210    28264      +54     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sug0
Copy link
Copy Markdown
Collaborator Author

sug0 commented Mar 14, 2025

thx tomas! I didn't remove the breaking:api tag manually, btw. I'm not sure what made it get removed on its own. 😕

sug0 and others added 8 commits March 14, 2025 10:16
This fix is not the most robust solution, but it is the one that
involves less code changes. Ideally, we would include the wrapper tx
hash in the native VP execution context, to compute the hash of the
inner transactions that is now being included in each wasm emitted
event, but this change is too invasive.
@sug0 sug0 force-pushed the tiago/inner-tx-event-attr branch from 2399ba4 to 7e7cd29 Compare March 14, 2025 10:20
@sug0
Copy link
Copy Markdown
Collaborator Author

sug0 commented Mar 14, 2025

(rebased on main)

@grarco grarco mentioned this pull request Mar 17, 2025
@brentstone brentstone added the merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass label Mar 18, 2025
mergify bot added a commit that referenced this pull request Mar 18, 2025
@mergify mergify bot merged commit 1343f1c into main Mar 18, 2025
26 of 28 checks passed
@mergify mergify bot deleted the tiago/inner-tx-event-attr branch March 18, 2025 16:51
@grarco grarco mentioned this pull request Mar 25, 2025
3 tasks
murisi added a commit that referenced this pull request Apr 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking:api public API breaking change enhancement New feature or request ledger merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants