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: fix signers #5942

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

simulate: fix signers #5942

wants to merge 24 commits into from

Conversation

joe-p
Copy link
Contributor

@joe-p joe-p commented Feb 23, 2024

Summary

This is a new feature in simulate that will close #5914. Auth address correction is done is two places:

  1. In simulateWithTracer the txn fields will be analyzed and auth addrs for txns will be updated past on previous rekeyTo fields
  2. In AfterProgram the auth addr field for all txns up to the next app call will be corrected

This allows one to simulate with empty signatures without knowing the proper auth addresses when forming the group.

Test Plan

Simple test to ensure the endpoint works as expected in test/e2e-go/restAPI/simulate/simulateRestAPI_test.go and more comprehensive testing in ledger/simulation/simulation_eval_test.go

@joe-p
Copy link
Contributor Author

joe-p commented Feb 23, 2024

@jasonpaulos let me know what you think about this general approach. If it looks good, I can work on the endpoint and more tests

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 direction makes sense to me. This will need extensive testing.

What are you thinking about for how to enable this? At the moment my preference would be a boolean alongside AllowEmptySignatures, which you'd only be allowed to set if AllowEmptySignatures is also set

ledger/simulation/simulator.go Outdated Show resolved Hide resolved
@joe-p
Copy link
Contributor Author

joe-p commented May 1, 2024

@jasonpaulos this comment in the test you initially wrote got me thinking... Should we expect the transaction group to be modified in the response? Is there any precedent for this?

I feel like there is value in seeing the txn group as it was given and also seeing what was modified. Perhaps an additional field that indicates a changed signer per txn, but the actual transaction object remains unchanged?

@joe-p joe-p changed the title simulate: fix auth addresses simulate: fix signers May 1, 2024
@joe-p
Copy link
Contributor Author

joe-p commented May 1, 2024

Also I'm switching from the terminology "fix auth addr" to "fix signer" to avoid confusion between the auth addr for a given account in the ledger and the signer for the transaction, making it clear the ledger remains unchanged and its just txns that are getting modified.

Copy link

codecov bot commented May 1, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 55.90%. Comparing base (f633857) to head (7dc3716).
Report is 1 commits behind head on master.

Files Patch % Lines
daemon/algod/api/server/v2/utils.go 0.00% 7 Missing ⚠️
ledger/simulation/simulator.go 83.72% 5 Missing and 2 partials ⚠️
ledger/simulation/tracer.go 80.95% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5942      +/-   ##
==========================================
+ Coverage   55.86%   55.90%   +0.04%     
==========================================
  Files         481      481              
  Lines       68357    68416      +59     
==========================================
+ Hits        38189    38251      +62     
+ Misses      27567    27565       -2     
+ Partials     2601     2600       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jasonpaulos
Copy link
Member

@jasonpaulos this comment in the test you initially wrote got me thinking... Should we expect the transaction group to be modified in the response? Is there any precedent for this?

I feel like there is value in seeing the txn group as it was given and also seeing what was modified. Perhaps an additional field that indicates a changed signer per txn, but the actual transaction object remains unchanged?

I agree that it would be preferable to not modify the transactions as given, and instead provide an additional response field with the overridden signer. The only problem is that the response structure may make it difficult to associate this additional information with inner transactions.

@joe-p
Copy link
Contributor Author

joe-p commented May 1, 2024

The only problem is that the response structure may make it difficult to associate this additional information with inner transactions.

Unless you are thinking of functionality that I am not, we're only modifying the signers on the outer transactions, so we only need to add additional information to the outers.

@jasonpaulos
Copy link
Member

Unless you are thinking of functionality that I am not, we're only modifying the signers on the outer transactions, so we only need to add additional information to the outers.

Good point, this plan should have no problems then

@joe-p joe-p marked this pull request as ready for review June 4, 2024 18:00
@joe-p
Copy link
Contributor Author

joe-p commented Jun 4, 2024

@jasonpaulos I need to modify the test case to make sure the change I made in 89c20b5 works as expected with nested app calls, but other than that it's ready for review. <- Done

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.

simulate fails with empty signature on a rekeyed account
3 participants