Fuzzing, Context Props and Recursive Proxies #208
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hayesgm
changed the title
hayesgm
force-pushed
the
hayesgm/fuzzing-rebased
branch
from
895cc0e
to
2f7e233
Compare
jflatow
reviewed
Mar 4, 2022
jflatow
reviewed
Mar 4, 2022
jflatow
reviewed
Mar 4, 2022
This was referenced Mar 4, 2022
Closed
jflatow
reviewed
Mar 4, 2022
Fix Issues with Forking and Snapshotting
This patch fixes issues with forking context and snapshotting. We ensure that forks actually do not conflict with one another by deep-cloning any necessary functions. Secondly, we improve snapshots so they are used between solutions of a solution-set. We probably need to work on what destructuring from CometContext looks like, which will be added to this branch before it's merged.
Context Properties
This patch creates "ContextProperties", which is a transformed version of context that is passed into scenarios. This allows us to not have to attach every possible value to the context object for destructuring, which becomes an issue when an object like `comet` changes (and thus should be received by an async getter), which either makes the scenario code more complicated (`let comet = await context.getComet()`), or we say `context.comet = comet` and then later if that value changes, we need to effectively observe those changes and reset this mirror-object. Instead, now, we create a function that is effectively: `getContextProperties(context) -> { comet: ..., ... }`. This function then can pull the data from the canonical source, but now the scenario can just destructure `{comet}` from this and not have to deal with guessing if this value is up to date (or awaiting it).
Additionally, we change `cache` to use `Map`s instead of objects for tracking its data and perform a smarter version of `deepClone`, which previously had a mistake that is was transforming `Map`s and other non-JSON data structures into flat objects, which then was failing further code. The new code deep clones Maps correctly and otherwise takes values as-is.
kevincheng96
approved these changes
Mar 7, 2022
hayesgm
force-pushed
the
hayesgm/fuzzing-rebased
branch
from
2f7e233
to
75cc7b5
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch is three separate patches together, subsuming #129 and #196.
Fuzzing
This patch adds fuzzing (originally #129) to scenarios. This allows us to add multiple runs for different random values when testing a property, e.g. if I supply 1 DAI or 100 DAI, do things still work. This will be fundamental in showing the properties are not just one-off special tests. H/T 🪖 for this work.
Context Properties
Context Properties simplify a difficult process in scenario contexts. It used to be that for a scenario, it would accept
scenario<C>(context: C, world: World), but this was difficult, since we often wanted to destructurescenario<C>({comet}: C, world: World), which means thatC(the context) needed a property (or getter) calledcomet, which needed to be updated with the Context. Meaning that if we wanted to use a new Comet context (e.g. because we deployed a new one in ModernConstraint), then we'd need to also callcontext.comet = newComet(and it would break if we didn't), and then when we wanted to fork, we needed to... undo this change? Plus, when migrating to DeploymentManager, these functions became often async (since we needed to read the ABI from disk). So it just became a mess. Also, if you wanted to call a function on the context, you needed to make sure thethiswas bound correctly, which is messy in JavaScript. Now, we add a function calledgetContextProperties<C, P>(context: C): Promise<P>which is called before every scenario and passed in as the first argument. This allows us to write:getContextProperties = function(context) { return { comet: await context.getComet() } }and thenscenario<C, P>({comet}: P, world: World, context: Context) { console.log(await comet.name()) }and it's guaranateed to work correctly. This makes life a ton easier when handling contexts, at the added code of adding a second generic param to manage.Recursive Proxies
We add support for recurisve proxies with merged ABIs. That is, if a contract like comet has a proxy
comet:implementation, and thencomet:implemenationhas a proxycomet:implementation:ext, now this can be picked up in spider and used in the deployed contract. This allows us to handle CometExt correctly in our scenarios.