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

Return Inner and Root transactions for nested logs support in C2C calls #820

Merged
merged 21 commits into from
Feb 10, 2022

Conversation

algochoi
Copy link
Contributor

@algochoi algochoi commented Jan 10, 2022

Summary

This PR includes some changes to support proper nested logs in inner transactions and adds/changes some tests to reflect this.

feature/contract-to-contract will now allow inner transactions to have appl calls. This means inner txns may also be recursive and many "levels" deep, as inner appl calls may have inner txns themselves.

Since logs are associated with each EvalDelta, logs returned by inner transactions would have to be recursively parsed or else the current API will return only the root txn's logs, even if the query matches an inner txn. This PR changes the transaction query to fetch for an inner txn if a returnInnerTxn flag in the filter is set to true so nested logs can be returned properly.

Test Plan

  • Changed some existing tests that check for inner txns to include nested appl calls.
  • Added an additional check to return the nested logs properly and check that non UTF-8s can be parsed correctly in inner asset/appl txns.
  • Added a test to check if inner logs are being returned properly.

Note: This PR also depends on the go-algorand feature/contract-to-contract branch due to nested inner txn logic.

@algochoi algochoi self-assigned this Jan 10, 2022
util/test/account_testutil.go Show resolved Hide resolved
api/converter_utils.go Outdated Show resolved Hide resolved
idb/postgres/postgres.go Outdated Show resolved Hide resolved
Add a null log in itxn

Fix tests

Fix inner asset id for cheeky test

Fix transaction query for inner assets
@algochoi algochoi marked this pull request as ready for review January 10, 2022 20:12
@algochoi algochoi changed the title WIP: Return Inner and Root transactions for nested logs support for C2C calls Return Inner and Root transactions for nested logs support in C2C calls Jan 10, 2022
Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

We should implement this with a new option to the transactions query to select whether or not to return the root transaction when matching an inner transaction. If you wrapped the LEFT OUTER JOIN in buildTransactionQuery with the new flag, I think most of the logic you have should work without changes.

Let me know if I'm missing something.

accounting/rewind.go Outdated Show resolved Hide resolved
idb/postgres/postgres.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2022

Codecov Report

Merging #820 (a9fd9e8) into develop (60f4b43) will increase coverage by 0.23%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #820      +/-   ##
===========================================
+ Coverage    58.87%   59.10%   +0.23%     
===========================================
  Files           36       36              
  Lines         4372     4382      +10     
===========================================
+ Hits          2574     2590      +16     
+ Misses        1484     1482       -2     
+ Partials       314      310       -4     
Impacted Files Coverage Δ
api/converter_utils.go 91.83% <100.00%> (+0.94%) ⬆️
api/handlers.go 71.10% <100.00%> (+0.12%) ⬆️
idb/idb.go 58.53% <100.00%> (+7.31%) ⬆️
idb/postgres/internal/writer/write_txn.go 79.38% <100.00%> (ø)
idb/postgres/postgres.go 49.18% <100.00%> (+0.13%) ⬆️

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 60f4b43...a9fd9e8. Read the comment docs.

@algochoi algochoi requested a review from winder January 27, 2022 16:07
Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, your changes look good. The only thing I see missing is a test that uses the new flag directly. Call db.Transactions with and without ReturnInnerTxnOnly and verify that the RootTxn is not returned when ReturnInnerTxnOnly is set.

@algochoi algochoi requested a review from winder January 28, 2022 20:10
@algochoi algochoi merged commit 2d47aa2 into develop Feb 10, 2022
@algochoi algochoi deleted the algochoi/c2c-indexer-changes branch July 11, 2022 20:03
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

3 participants