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

Implement tx counter for transaction info #621

Merged
merged 3 commits into from
Sep 27, 2021
Merged

Implement tx counter for transaction info #621

merged 3 commits into from
Sep 27, 2021

Conversation

alpe
Copy link
Contributor

@alpe alpe commented Sep 27, 2021

Resolves #601

Based on the #608 spike

Changes:

  • Use uint32 for the tx counter
  • Sets wasmvm TransactionInfo
  • Non default 0 counter but empty TransactionInfo on simulations
  • Tests
  • Better doc

@alpe alpe mentioned this pull request Sep 27, 2021
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Nice clean version.

👍

app/ante.go Outdated
ante.NewSigGasConsumeDecorator(ak, sigGasConsumer),
ante.NewSigVerificationDecorator(ak, signModeHandler),
ante.NewIncrementSequenceDecorator(ak),
wasmkeeper.NewCountTXDecorator(txCounterStoreKey),
Copy link
Member

Choose a reason for hiding this comment

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

We should add some note that chains using x/wasm need to add this custom AnteHandler if they want Tx Index info.

Not sure where that goes? Integration.md?

@ethanfrey
Copy link
Member

Please make sure to tick the box in #618 after merging

@codecov
Copy link

codecov bot commented Sep 27, 2021

Codecov Report

Merging #621 (a31f40f) into master (fc8d33e) will increase coverage by 0.34%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #621      +/-   ##
==========================================
+ Coverage   60.04%   60.39%   +0.34%     
==========================================
  Files          45       48       +3     
  Lines        5209     5254      +45     
==========================================
+ Hits         3128     3173      +45     
  Misses       1859     1859              
  Partials      222      222              
Impacted Files Coverage Δ
x/wasm/types/keys.go 34.88% <ø> (ø)
app/ante.go 100.00% <100.00%> (ø)
app/app.go 84.01% <100.00%> (ø)
x/wasm/keeper/ante.go 100.00% <100.00%> (ø)
x/wasm/types/ante.go 100.00% <100.00%> (ø)
x/wasm/types/types.go 52.20% <100.00%> (+0.60%) ⬆️

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -63,6 +63,10 @@ from the Cosmos SDK, and enabled them in `app.go`. If so, you can just look
at [`wasmd/app/app.go`](https://github.com/CosmWasm/wasmd/blob/master/app/app.go#)
for how to do so (just search there for lines with `wasm`).

`wasmd` also comes with a custom `ante handler` that adds the TX position in the block into the context
Copy link
Member

Choose a reason for hiding this comment

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

Nice

@@ -8,7 +8,7 @@ echo "## Add new CosmWasm contract"
RESP=$(wasmd tx wasm store "$DIR/../../x/wasm/keeper/testdata/hackatom.wasm" \
--from validator --gas 1500000 -y --chain-id=testing --node=http://localhost:26657 -b block)

CODE_ID=$(echo "$RESP" | jq -r '.logs[0].events[0].attributes[-1].value')
CODE_ID=$(echo "$RESP" | jq -r '.logs[0].events[1].attributes[-1].value')
Copy link
Member

Choose a reason for hiding this comment

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

The ante handler added an event?
Or this is just an older bug you just found?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a 🐛 that I found with manual tests. Not related to the ante handler.

@alpe alpe merged commit 58f3776 into master Sep 27, 2021
@alpe alpe deleted the 601-tx_counter branch September 27, 2021 19:53
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.

Add transaction index implemented as counter
2 participants