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

Simulate endpoint tests #265

Merged
merged 18 commits into from
Mar 13, 2023
Merged

Simulate endpoint tests #265

merged 18 commits into from
Mar 13, 2023

Conversation

algochoi
Copy link
Contributor

@algochoi algochoi commented Feb 24, 2023

Adds tests for:

  • Calling the simulate endpoint for basic payment txns
  • Using the simulate method/functionality within an AtomicTransactionComposer
  • Check fail-path for inner transaction failures
  • Simulating unsigned transactions

Also changes the following (not-necessarily related to simulate)

  • Decrease transient account funding to reduce economic burden on genesis accounts
  • Add a method in RandomByte TEAL contract to call a bad inner txn

Closes #270

Related SDK PRs

.env Outdated Show resolved Hide resolved
.env Outdated Show resolved Hide resolved
@algochoi algochoi changed the title [Draft] Simulate endpoint tests Simulate endpoint tests Mar 1, 2023
@algochoi algochoi marked this pull request as ready for review March 1, 2023 20:14
Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

Looking solid. My main question surrounds what to do (if anything) about simulating without signatures. My intuition is that we should support it. But if we do, then we also need to make it easy to send unsigned transactions and also to collect the results and have some method such as would_have_succeeded_but_for_signatures() (please find a better name) that easily answers the question suggested by this terrible method name.

features/integration/abi.feature Show resolved Hide resolved
features/integration/simulate.feature Outdated Show resolved Hide resolved
features/integration/simulate.feature Show resolved Hide resolved
features/integration/simulate.feature Show resolved Hide resolved
features/integration/simulate.feature Outdated Show resolved Hide resolved
features/integration/simulate.feature Show resolved Hide resolved
| 1234523 | X4Bl4wQ9rCo= |

@simulate
Scenario: Simulating duplicate transactions in a group
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the above, if we want to support signature-missing simulations with the AtomicTransactionComposer, its API would need to support this and the feature tested.

features/integration/simulate.feature Outdated Show resolved Hide resolved
@algochoi algochoi requested a review from tzaffi March 3, 2023 15:13
Given default transaction with parameters <amt> "<note>"
When I get the private key
And I sign the transaction with the private key
And I simulate the transaction
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it's worth distinguishing between simulating with signed transactions and simulating without ?

If so, this step could instead by:

And I simulate-signed the transaction

leaving room for the future

And I simulate-unsigned the transaction

But if the API is going to be the same (i.e. simulate just takes the transactions as they are and doesn't need special methods to handle unsigned), then we don't need to change this step.

We could also punt this to #267 and if the API changes at a later date, we would change the steps as well - though that would be a bit annoying.

Copy link
Contributor Author

@algochoi algochoi Mar 3, 2023

Choose a reason for hiding this comment

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

I don't think the API would change, so no need for two separate steps.

Currently, the simulate endpoint itself sends the msgpack encoded transaction (bytearray-like object). As a part of #267 , we probably need helper methods to encode unsigned transactions into msgpack, and then send that to the simulate endpoint. Currently, we have explicit checks in the SDKs to look for signatures before doing this encoding.

When I build a payment transaction with sender "transient", receiver "transient", amount 100001, close remainder to ""
And I create a transaction with signer with the current transaction.
And I add the current transaction with signer to the composer.
Then I simulate the current transaction group with the composer
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar question. Should we anticipate the following step, and therefore rename this one?

Then I simulate-signed the current transaction group with the composer

Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

Just two remaining questions: whether source is needed as channel in .env and whether our steps should assume that simulate-signed will have a different API than simulate-unsigned.

Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

LGTM

* Add unsigned transaction simulation tests

* Add another test case for multiple unsigned txns
@algochoi
Copy link
Contributor Author

algochoi commented Mar 9, 2023

@tzaffi I added new tests for simulating unsigned transactions a1023f8 - let me know if you have any feedback!

@algochoi algochoi merged commit ef5b982 into master Mar 13, 2023
@algochoi algochoi deleted the simulate-endpoint-tests branch March 13, 2023 16:56
@algochoi
Copy link
Contributor Author

Since we merged algorand/py-algorand-sdk#420 and algorand/py-algorand-sdk#445 in, it makes sense to merge this in as well.

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