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

analyzer/runtime: Deposit/Withdraw txs: make "to" explicit #645

Merged
merged 2 commits into from
Feb 20, 2024

Conversation

mitjat
Copy link
Contributor

@mitjat mitjat commented Feb 20, 2024

This PR:

  • makes sure the to field of a tx (in the DB and the API) is always populated
  • adds an initial unit(ish) test for the runtime analyzer, with the ambition that more will follow in future PRs

Background:
For txs with method consensus.Deposit or consensus.Withdraw, the to field in the body is optional; when omitted, the implied semantics are that the deposit/withdrawal is happening to the account of the tx sender.

Nexus's /transactions endpoint lists txs with their raw body (where we don't want to inject the implicit to), but also with an additional to field that we extract from txs regardless of the method. This PR makes it so that this field is always filled, even if to in the tx body is missing.

Testing:

  • Manual. I indexed round 316043 from pontusx that is known to contain a tx with an implicit to, and verified the API response.
  • The included test.
  • The regression e2e range unfortunately does not include any deposits with an implicit to, so the change is not visible there :(

@mitjat mitjat force-pushed the mitjat/txs-explicit-to branch 3 times, most recently from 197a8f6 to ff74949 Compare February 20, 2024 22:33
@pro-wh
Copy link
Collaborator

pro-wh commented Feb 20, 2024

is this started from another branch?

@pro-wh
Copy link
Collaborator

pro-wh commented Feb 20, 2024

lg, cool tests

@mitjat mitjat force-pushed the mitjat/txs-explicit-to branch from ff74949 to 4b85a8f Compare February 20, 2024 22:57
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 cannot fully decide if this is the right abstraction level. We could narrow the scope of tests and analyze the internal variables that the runtime analyzer populates during analysis, and then uses them to generate DB statements. The upside of that would be a more focused test. The downside would be having to treat the analyzer a lot more as a white box. Another downside is that it would make it harder to port these tests to consensus: The consensus analyzer doesn't have that convenient intermediate in-memory representation, it produces DB statements more directly from node data.
So I decide to go with this, and tweak/refactor it later if needed. 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated changes; I just found the old wording slightly misleading when debugging pontusx.

@mitjat mitjat merged commit e86b4db into main Feb 20, 2024
9 checks passed
@mitjat mitjat deleted the mitjat/txs-explicit-to branch February 20, 2024 23:34
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.

3 participants