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

tools: Support logs and Inner transactions in tealdbg #3547

Merged
merged 86 commits into from
Feb 17, 2022

Conversation

algochoi
Copy link
Contributor

@algochoi algochoi commented Feb 1, 2022

Summary

This PR adds logs as an array and one layer of inner txns as an object in the local scope of tealdbg. The logs are represented as a []string and inner txns as []transactions.SignedTxnWithAD in AppState. Currently this PR is based on the feature/contract-to-contract branch to support nested inner txns.

Short explanation

  • User wants to look at the logs/itxns, makeLocalScope() is called and the logs/itxns array is created with their respective object IDs. The object IDs map to makeLogsState() and makeInnerTxnsState().
  • When logs and itxns are encoded, the group ID is appended at the end of the object ID, e.g. innerTxnObjID_id_01_02_03 denotes s.innerTxn[1][2][3] by supplying an array of group IDs. For a single inner transaction, it uses the innerTxnObjID_id prefix and for an array of nested inner transactions, it is encoded as innerTxnObjID_nested.
  • When the ID is decoded, traverse to the inner transaction. If the ID is a single inner transaction, then call makeInnerTxnImpl(), build the transaction object and encode the log obj and array of nested inner transactions ID. If the ID is an array of inner transactions, then create a slice of inner transaction objects and encode each inner transaction object by appending the group ID.

Test Plan

Visually checked output and updated test case. There is a script for generating a test transaction and debugging context here.
Screen Shot 2022-02-03 at 10 44 58 PM

For any interested observers, there is an example test case with nested itxns you can run on the browser in this repo. You can do something like:

  • checkout this branch then make install
  • run tealdbg from your GOPATH bin with the example dryrun msgp, e.g. ./tealdbg debug path-to-test-repo/itxn_call2.teal -d path-to-test-repo/generated-data/dryrun_txn.msgp --group-index 5

jannotti and others added 30 commits October 6, 2021 20:49
…d#3237)

* Three new globals for to help contract-to-contract usability

* detritis

* Check error

* doc comments
* Three new globals for to help contract-to-contract usability

* detritis

* Check error

* doc comments

* opcode, docs, and tests

* specs update
@algochoi algochoi changed the title WIP: Support logs and Inner transactions in tealdbg Support logs and Inner transactions in tealdbg Feb 10, 2022
@algochoi
Copy link
Contributor Author

I tested locally and this looks pretty good! I just have some questions/comments.

  1. This PR does not implement "step into" for c2c calls. Is there a separate issue for that?

I think this issue addresses the step into: https://github.com/algorand/go-algorand-internal/issues/1789

  1. Inner transactions have all of the "normal" transaction fields displayed, but they don't have any "effect" fields displayed besides logs. Ideally you would be able to access CreatedAssetID and CreatedApplicationID as well.

Good point - I can try to address this in this PR 👍

  1. I noticed that the inner transactions and logs arrays in transactions are hidden if they are 0. Personally I'd prefer them shown as 0-length arrays similar to other txn array fields, but this is a minor thing.

I'll play around with this and see what works better - I initially moved it out because the sidebar looked a bit congested, but I think that's a fair point.

  1. Also, not sure if this is appropriate to address here, but the global field OpcodeBudget does not update after every op like I would expect. I think it's because the debugger assumes global fields never change, which up until now was true.

True, I'll play around with this as well - I will address this in this PR or create an issue to address this.

@algochoi algochoi changed the base branch from feature/contract-to-contract to master February 10, 2022 22:16
@algochoi algochoi changed the base branch from master to feature/contract-to-contract February 10, 2022 22:18
@algochoi algochoi changed the base branch from feature/contract-to-contract to master February 10, 2022 22:30
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.

The budget changes look great!

I have one last suggestion that should not matter practically, but it's probably a good idea

cmd/tealdbg/cdtState.go Outdated Show resolved Hide resolved
@algochoi algochoi marked this pull request as ready for review February 16, 2022 23:42
jasonpaulos
jasonpaulos previously approved these changes Feb 17, 2022
cmd/tealdbg/cdtState.go Outdated Show resolved Hide resolved
cmd/tealdbg/local.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.

One last remark

cmd/tealdbg/cdtState.go Outdated Show resolved Hide resolved
algorandskiy
algorandskiy previously approved these changes Feb 17, 2022
cmd/tealdbg/cdtSession.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

Approved (Pavel also had before the last commit removed it.)

@jannotti jannotti merged commit fb1d9c0 into algorand:master Feb 17, 2022
@onetechnical onetechnical changed the title Support logs and Inner transactions in tealdbg tools: Support logs and Inner transactions in tealdbg Feb 17, 2022
@algochoi algochoi deleted the algochoi/tealdbg-itxns branch January 12, 2023 16:02
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.

None yet

8 participants