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

Introduce itxn to get inner transaction results #2883

Merged
merged 32 commits into from
Sep 14, 2021
Merged

Conversation

jannotti
Copy link
Contributor

This allows axfer and afrz, and adds itxn so that the new asset id can be read.

jannotti and others added 29 commits September 8, 2021 20:57
This only changes the blocks, so there is still a lot of work to do so
that external APIs see things properly.  The REST API assumes that it
can determine a creatble ID based on the index of a transaction in a
block and the block's TxnCounter.  That is no longer true, it must
actually figure out how many inner txns it is skipping over.
Co-authored-by: Jason Paulos <jasonpaulos@users.noreply.github.com>
This also sorts out inner txcounting, so that an innner transaction
knows the current txncount as it executes, so it can get the right id
if it creates.
@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2021

Codecov Report

Merging #2883 (363445a) into master (0cfe715) will decrease coverage by 4.34%.
The diff coverage is 43.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2883      +/-   ##
==========================================
- Coverage   47.28%   42.94%   -4.35%     
==========================================
  Files         355      353       -2     
  Lines       56901    57177     +276     
==========================================
- Hits        26908    24552    -2356     
- Misses      26958    29616    +2658     
+ Partials     3035     3009      -26     
Impacted Files Coverage Δ
config/consensus.go 84.28% <ø> (ø)
daemon/algod/api/server/v1/handlers/handlers.go 0.62% <0.00%> (-0.01%) ⬇️
daemon/algod/api/server/v2/utils.go 13.86% <0.00%> (-0.87%) ⬇️
data/pools/transactionPool.go 41.43% <0.00%> (-3.07%) ⬇️
data/transactions/logic/doc.go 77.41% <0.00%> (-5.34%) ⬇️
data/transactions/logic/fields.go 81.57% <ø> (ø)
data/transactions/logic/fields_string.go 5.03% <0.00%> (-0.15%) ⬇️
data/transactions/logic/opcodes.go 100.00% <ø> (ø)
data/transactions/transaction.go 28.74% <0.00%> (-3.77%) ⬇️
data/transactions/verify/txn.go 43.42% <ø> (-0.88%) ⬇️
... and 115 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cfe715...363445a. Read the comment docs.

@onetechnical
Copy link
Contributor

onetechnical commented Sep 14, 2021

FYI: the required CircleCI tetsts clearly passed, as did all the other required tests, so I'm not sure why this is reporting that checks haven't completed yet. I would consider this a passed tests result, at least as of commit 24c76af.

data/transactions/logic/assembler.go Outdated Show resolved Hide resolved
data/transactions/logic/doc.go Outdated Show resolved Hide resolved
data/transactions/logic/fields.go Outdated Show resolved Hide resolved
data/transactions/logic/eval.go Outdated Show resolved Hide resolved
data/transactions/logic/eval.go Show resolved Hide resolved
data/transactions/logic/eval.go Show resolved Hide resolved
ledger/apptxn_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

Looks good, few comments

data/transactions/logic/README.md Outdated Show resolved Hide resolved
data/transactions/logic/assembler_test.go Outdated Show resolved Hide resolved
data/transactions/logic/fields.go Outdated Show resolved Hide resolved
data/transactions/logic/eval.go Show resolved Hide resolved
data/transactions/logic/eval.go Show resolved Hide resolved
data/transactions/transaction.go Show resolved Hide resolved
tx_field Receiver
tx_submit
itxn_field Receiver
itxn_submit
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if it fits but consider adding new itxn opcode into e2e test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do one in the integration test (not true e2e). I will add a e2e sub test while in betanet (after making it so I can use python!)

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, these e2e_subs tests are shared with the indexer so it is worth having new opcodes/side effects here

jasonpaulos
jasonpaulos previously approved these changes Sep 14, 2021
Copy link
Member

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Looks good!

@jannotti jannotti merged commit 08d31fa into algorand:master Sep 14, 2021
@jannotti jannotti deleted the itxn branch January 28, 2022 15:39
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.

None yet

6 participants