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

Parse: Tenderly Simulation Response #228

Merged
merged 2 commits into from
Apr 6, 2023

Conversation

bh2smith
Copy link
Contributor

@bh2smith bh2smith commented Apr 5, 2023

Part of #177

In #227 we introduced a transaction simulation interface and implementation via Tenderly. This PR extends on the functionality by parsing only the relevant fields of the simulation results. In essence,

  • introduce another abstract method to the TxSimulator interface (parseSimulation). This functionality is separated so that we can unit test parsing without requiring an API call.
  • provide a concrete implementation of parseSimulation and test that it works as intended.

@bh2smith bh2smith mentioned this pull request Apr 5, 2023
40 tasks
@bh2smith bh2smith force-pushed the parse-settlement-simulation branch from 0231a52 to e10c0e9 Compare April 5, 2023 09:23
@bh2smith bh2smith force-pushed the simulate-settlement branch from e6de2ec to 812c1e8 Compare April 5, 2023 11:22
@bh2smith bh2smith force-pushed the parse-settlement-simulation branch from cc3e804 to 5eb431b Compare April 5, 2023 11:30
@bh2smith bh2smith requested a review from a team April 5, 2023 11:30
@bh2smith bh2smith changed the title Parse Tenderly Simulation Parse: Tenderly Simulation Response Apr 5, 2023
@bh2smith bh2smith marked this pull request as ready for review April 5, 2023 13:01
@bh2smith bh2smith force-pushed the parse-settlement-simulation branch from 72422bd to 53fa626 Compare April 6, 2023 10:26
* Converts simulation result into `SimulationData` containing only the relevant fields.
* @param simulation - return value from simulate method.
*/
abstract parseSimulation(simulation: any): SimulationData;
Copy link

Choose a reason for hiding this comment

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

it would be great if the param is not of type any

Copy link

Choose a reason for hiding this comment

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

I don't think this method belong to the abstract class.

It should be a static util, used by Tenderly implementation of the interface I suggested in #227

Instead, the method simulate should return Promise if this is the only data you care about. So you can start with simpler closed interfaces, and add data to them based on need.

If in the future you need more data in SimulationData you just add it, and then the static function i suggested above will require to implement the extraction of the data from the Tenderly result

Copy link

@anxolin anxolin Apr 6, 2023

Choose a reason for hiding this comment

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

BTW, parse is usually applied if you are dividing some string, bytes and extracting some logical parts from it.

Here you are technically converging one object into another one. So i don't think you are parsing anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Parse" is to resolve something into its parts. I would say that this is exactly what we are doing here. Pruning irrelevant pieces and interpreting the response.

blockNumber: number;
// Event Logs emitted within the transaction's simulation.
logs: EventLog[];
}
Copy link

Choose a reason for hiding this comment

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

im a bit confused about the structure of the project. I see a pythion project with a nodejs project inside a folder (internal_tranfer/actions). And then there's models in the src but also in src/simulate

For me, SimulationData should be part of the interface of the method parseSimulation so it should be in the same file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea. I just don't understand how to put function signatures into an interface.. until now I have only seen interfaces used to define data structures (without functionality).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

im a bit confused about the structure of the project. I see a pythion project with a nodejs project inside a folder (internal_tranfer/actions).

Yes, what you are seeing is what it is.

And then there's models in the src but also in src/simulate

models inside of simulate are related only to the simulation abstraction (I thought this would keep simulation self contained). I can also easily move SimulationData into the same file (no problem).

@bh2smith
Copy link
Contributor Author

bh2smith commented Apr 6, 2023

Merging into base because they belong together cc @anxolin

@bh2smith bh2smith merged commit f1b30e7 into simulate-settlement Apr 6, 2023
@bh2smith bh2smith deleted the parse-settlement-simulation branch April 6, 2023 11:48
@github-actions github-actions bot locked and limited conversation to collaborators Apr 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants