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

Action Models #182

Merged
merged 8 commits into from
Mar 28, 2023
Merged

Action Models #182

merged 8 commits into from
Mar 28, 2023

Conversation

bh2smith
Copy link
Contributor

@bh2smith bh2smith commented Feb 8, 2023

To extend our current web3 action, we prepare the foundational modeling and parsing logic behind processing a Settlement Transaction. In particular this PR introduces

Part of #177

Models

  • TransferEvent, TradeEvent and SettlementEventcontaining only the foreseeable relevant information for Slippage accounting purposes (e.g. TradeEvent only contains owner) which will be used to classify transfers as AMM_{IN/OUT} vs USER_{IN/OUT}

Utilities

  • Parse Address from bytes32 (which converts event topics like 0x0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 into addresses like 0x9008d19f58aabd9ed0d60971565aa8510560ab41
  • TransferEvent.involves(address) used to filter out only relevant transfers during transaction processing.

Functionality

  • partitionEventLogsa method which transforms transaction event logs into the relevant events defined above.

Test Plan

Will add CI that runs all the new tests.

Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

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

Looks good (would have been nice to split out testing setup and CI logic from the rest of the code).
I believe most of the parsing code could be replaced by depending on the contracts npm package und using ethers parsing library (which I'd prefer a lot) but isn't mandatory.

internal_transfers/actions/index.ts Outdated Show resolved Hide resolved
.github/workflows/pull-request.yaml Show resolved Hide resolved
internal_transfers/actions/src/models.ts Outdated Show resolved Hide resolved
internal_transfers/actions/src/parse.ts Outdated Show resolved Hide resolved
Base automatically changed from web3-action to slippage-v2 February 9, 2023 10:07
Copy link
Contributor

@fedgiac fedgiac left a comment

Choose a reason for hiding this comment

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

The code works, but I also prefer to use a library rather than doing manual event parsing.

internal_transfers/actions/tests/parse.spec.ts Outdated Show resolved Hide resolved
internal_transfers/actions/src/utils.ts Outdated Show resolved Hide resolved
@bh2smith
Copy link
Contributor Author

bh2smith commented Feb 9, 2023

The code works, but I also prefer to use a library rather than doing manual event parsing.

I added an issue for this already. I'm not sure it makes sense to add all that into this PR, but rather as an improvement later. See here

#186

What do you think? Now or later?

@bh2smith bh2smith requested a review from a team February 13, 2023 09:19
Copy link
Contributor

@gentrexha gentrexha left a comment

Choose a reason for hiding this comment

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

I left some nitpicky discussion comments and agreed with some of the issues raised.

Other than that, LGTM.

bh2smith and others added 3 commits March 27, 2023 15:30
Introducing transaction log parsing via ethers library with appropriate Contract artifacts. Namely, we introduce dependencies on @cowprotocol/contracts which contain the abi files for GPv2Settlement and iERC20 and suffice for us to decode relevant event data from settlement transactions. For us these are Transfer, Settlement and Trade.

We construct Event types out of each which only keep the parts that matter for our purposes (namely TradeEvent having owner as the only relevant field).
@github-actions
Copy link

github-actions bot commented Mar 27, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@socket-security
Copy link

New dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

Pull request alert summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package Capability Access +/- Transitive Count Publisher
@cowprotocol/contracts@1.3.2 network +7 cowprotocol_dev
ethers@6.0.3 network, filesystem +6 ricmoo
ts-jest@29.0.5 filesystem, environment +280 kul
eslint@8.27.0 filesystem, environment +96 eslintbot
jest@29.4.1 None +276 simenb
@types/jest@29.4.0 None +49 types
prettier@2.7.1 eval, filesystem, environment +0 prettier-bot

@bh2smith
Copy link
Contributor Author

@fedgiac - I have addressed your concerns in the PR #193 - which has since been merged here. Would you like to take another look?

@bh2smith bh2smith requested a review from fleupold March 27, 2023 15:36
@bh2smith bh2smith merged commit a146af8 into slippage-v2 Mar 28, 2023
@bh2smith bh2smith deleted the action-models branch March 28, 2023 11:29
@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2023
@fedgiac
Copy link
Contributor

fedgiac commented Mar 28, 2023

A bit late, but it looks good to me!

As an improvement I'd suggest to check that the emitter address is the settlement contract for events where settlementEventLog != null.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants