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: Allow unnamed foreign resource access #5366

Merged

Conversation

jasonpaulos
Copy link
Contributor

@jasonpaulos jasonpaulos commented May 8, 2023

Summary

Allows unnamed resources to be referenced from simulated transaction groups. The amount of unnamed resources is (mostly) limited by the number of additional resources the group could reference, but chooses to omit.

For example, a group with completely full foreign refs won't be able to use any unnamed references, and a group with no foreign refs declared will be able to use the maximum amount.

Still to do:

  • Cross-product resources (asset holdings and app local states) need to be limited beyond just the constraints on the individual component resources.
    • Tests for this
    • Test to ensure we don't count cross-products with newly created apps/assets against limits
  • Keep track of box read/write quota and report empty box refs if needed (per Simulate: Allow unnamed foreign resource access #5366 (comment))
  • REST API responses need to include the referenced unnamed resources.
    • Sort the REST response arrays to make responses deterministic, for easier testing. Skipped for now, since this may not be necessary
  • If a txn group has fewer than 16 txns, we could assume the remaining txns are app calls with 0 referenced resources. This would raise the amount of unnamed resources allowed while still being within an achievable amount.
  • Add more tests around unnamed resource limits.

Test Plan

Unit tests added for the logic package and simulation. E2E tests also added for goal and simulation REST API.

Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

I am taking a last look at this PR, closing to an end. Not trying to stop anything, just asking some questions.

ledger/simulation/resources.go Show resolved Hide resolved
ahangsu
ahangsu previously approved these changes Aug 7, 2023
Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

The only thing I was concerning while I was reading this PR is inner-call from v9 to v8, which I failed to catch up on a live discussion. By reading thread #5366 (comment) it seems more clear to me that the intention is better suited for pure-post-v8 txn group, if I understood correctly.

jannotti
jannotti previously approved these changes Aug 8, 2023
ahangsu
ahangsu previously approved these changes Aug 9, 2023
@jasonpaulos jasonpaulos dismissed stale reviews from ahangsu and jannotti via 6892ffc August 9, 2023 16:17
@jasonpaulos jasonpaulos closed this Aug 9, 2023
@jasonpaulos jasonpaulos reopened this Aug 9, 2023
@ahangsu ahangsu closed this Aug 9, 2023
@ahangsu ahangsu reopened this Aug 9, 2023
@ahangsu
Copy link
Contributor

ahangsu commented Aug 9, 2023

sry dude, was trying to get license cla pass through...

@bbroder-algo bbroder-algo merged commit 1088a2a into algorand:master Aug 9, 2023
24 checks passed
@jasonpaulos jasonpaulos deleted the simulate-unlimited-indirection branch August 9, 2023 20:16
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.

5 participants