Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

Allow to append logs after a post processing hook #1025

Merged
merged 3 commits into from
Apr 5, 2022

Conversation

thomas-nguy
Copy link
Contributor

@thomas-nguy thomas-nguy commented Apr 1, 2022

Closes: #1024

Description

Add the ability to append log to the receipt after a tx post processing hook

more information on the issue ticket.

open for discussion


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Apr 1, 2022

Codecov Report

Merging #1025 (8d751c6) into main (8202a09) will decrease coverage by 0.00%.
The diff coverage is 82.14%.

❗ Current head 8d751c6 differs from pull request most recent head 711ae62. Consider uploading reports for the commit 711ae62 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1025      +/-   ##
==========================================
- Coverage   59.16%   59.15%   -0.01%     
==========================================
  Files          80       80              
  Lines        6577     6576       -1     
==========================================
- Hits         3891     3890       -1     
  Misses       2467     2467              
  Partials      219      219              
Impacted Files Coverage Δ
x/evm/keeper/state_transition.go 75.54% <82.14%> (-0.08%) ⬇️

Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Can't you just modify the logs from the receipt?

@thomas-nguy
Copy link
Contributor Author

Yes, but the computation of the bloom needs to be moved down nevertheless.

Do you prefer to have this ability implicitly and keep the previous signature?

@thomas-nguy thomas-nguy force-pushed the thomas/add-log-to-hook branch from e4ba2bd to 0b02641 Compare April 4, 2022 10:28
@fedekunze
Copy link
Contributor

Yes, but the computation of the bloom needs to be moved down nevertheless.

Why? if you are updating the logs then you should also be updating the bloom during the post tx processing step. We'd need to change the state transition to do the following instead:

	if len(receipt.Logs) > 0 {
		// Update transient block bloom filter
		k.SetBlockBloomTransient(ctx, receipt.Bloom)
		k.SetLogSizeTransient(ctx, uint64(txConfig.LogIndex)+uint64(len(receipt.Logs)))
	}

@thomas-nguy thomas-nguy force-pushed the thomas/add-log-to-hook branch from 120b4c5 to 1cf2f36 Compare April 5, 2022 03:09
@thomas-nguy thomas-nguy changed the title Add the ability to append additional log in the receipt after a post processing hook Allow to append logs after a post processing hook Apr 5, 2022
@thomas-nguy
Copy link
Contributor Author

okay, make sense. Published a minimal change version

add changelog

fix tests

rename variable

minimum change version
@thomas-nguy thomas-nguy force-pushed the thomas/add-log-to-hook branch from 1cf2f36 to f04c02a Compare April 5, 2022 03:22
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM

x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
@fedekunze fedekunze enabled auto-merge (squash) April 5, 2022 15:33
@fedekunze fedekunze requested a review from crypto-facs April 5, 2022 15:33
@fedekunze fedekunze merged commit 56c4a31 into evmos:main Apr 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the ability to append additional log in the receipt after a post processing hook
2 participants