simulate: resource population #6015
Conversation
joe-p
force-pushed
the
feat/populate_resources
branch
from
466fd50 to
5ba0a9a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6015 +/- ##
==========================================
+ Coverage 51.60% 51.78% +0.18%
==========================================
Files 649 649
Lines 87048 87418 +370
==========================================
+ Hits 44917 45267 +350
- Misses 39269 39287 +18
- Partials 2862 2864 +2 ☔ View full report in Codecov by Sentry. |
|
All comments have been addressed. I also noticed CI failed. I need to step away for a minute but will take a closer look later today. Also apologies for the long delays between addressing feedback, should be able to focus on this to get it across the finish line now. Edit: Seemed to have introduced a regression |
This was erroneously removed in 6353f3b
|
You probably need to re-merge master and |
There was a problem hiding this comment.
Some simplistic code comments, and some question about when you add certain things. I think the existing order may be perfectly fine, despite my early comments reservations. But take my confusion as a request to find a good place to describe the whole strategy all at once in a comment.
I became worried that you are not accounting for old avm version apps, which can't use resource sharing. But I think maybe you're ok, because the response from simulate was taken that into account and places the necessary refs in the specific transactions if needed?
| Boxes: &boxes, | ||
| } | ||
|
|
||
| return &populatedResourceArrays |
There was a problem hiding this comment.
I'm concerned this will always return in the json even if it's empty. It will always be non-nil, and it will always contain non-nil fields. boxes is always non-nil, and the others probably are too, depending on exactly how convertSlice deals with empty slices.
| return nil | ||
| } | ||
|
|
||
| // addBox adds a box to the box array. It does NOT add the app to the app array. |
There was a problem hiding this comment.
Perhaps it should fail if the app is not there then? I haven't looked to see how it's used yet...
| }) | ||
| } | ||
|
|
||
| // Then assign accounts because they have a lower limit than other resources |
There was a problem hiding this comment.
Ok, interesting. You're doing the single resources last. In some of my reasoning above, I assumed you'd do them first. I think I agree with this though. Your cross-ref types will "find" the best place to land based on the existing references and fields, right? So after that, it is indeed probably better to do these last, because the cross-ref types may make have already put them somewhere.
I will add this in a comment but the general philosophy is to go in order of resources with the most restrictions to least. This is why the order is
Yeah exactly. The logic for transaction-specific references exists upstream in simulate. By the time we reach the resource populator we know txn-specific resources and can handle those first |
App IDs from simulate will always be hydrated, but we need a function for adding "empty" box refs
|
We've gotten so close to this being ready, but I wonder if the work in #6286 means we should hold off on this. The access list will dramatically simplify resource population so not entirely sure if it's worth merging in this fairly complex algorithm when we know we have a more elegant solution around the corner. Any thoughts? |
|
Id say it depends entirely on how soon the access PR gets merged |
|
My expectation is that there will be a go-algorand release in the next week or so that does not have #6286 (and is not a consensus release) and then there will be a consensus release that has tx.Access next, maybe at the end of July. I'm not sure what the best recommendation is for this code. I could imagine it staying, to support pre-sharing apps, or the very rare cases where the "cross-product" nature of foreign arrays allows a transaction to access more than tx.Access. But I'm not sure the complexity of supporting both (and choosing between them intelligently?) forever is worth it. |
|
Supporting resource population just for App post |
Yeah I think this is the main thing to consider.
Population for pre-sharing apps is already trivial because the existing simulate response already gives you unnamed resources per txn. Once we have the access list there will basically be no actual use-case for this code, so I don't think it makes sense to merge it in. As such, closing this PR. Will re-evaluate whether we want this functionality in algod for access list and make another PR if so. |
Summary
When a user calls simulate with
UnnamedResourcesenabled, simulate should suggest to the user how they can populate the resource arrays in their transactions to properly send the transaction group to the network.Test Plan
/simulateendpoint with ResourcePopulator functionalityledger/simulation/resources.gocoverage