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

Use SDK to poll and create smart order and handle result #16

Merged
merged 23 commits into from
Aug 30, 2023

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Aug 24, 2023

Poll logic

Incorporate all the poll logic from the SDK

Why the logic is being moved to the SDK?

It is great to delegate to the SDK because:

Auth Check in the SDK + Don't try again logic

The PR incorporate some handling of the poll result. Like when we don't need to try again to index an order.

Additionally, now by delegating to the SDK, we check out of the box if the order has been authenticated, and prints a nice message:

image

The watch tower will understand that this order doesn't need to be re-attempted in the future.

In order to test this, you can create an order and cancel it. Or you could index this block number:

BLOCK_NUMBER=29707125 
NETWORK=100

In case of success

This version will still successfully post the order to the API:
image

The only difference is that now, the responsibility of who is returning the discrete order details is in the SDK.

Network is not a String anymore

This PR changes the network from a String to a SupportedChainId. Since we use the SDK, it is nice we use its types.

Sorry for the noise on the chain.

One place where I didn't change it, is in the Registry that is persisted. I haven't update there yet to avoid making an incompatible chain in this PR. I prefer to avoid migrating the model for this.

To reproduce add this to your env

NETWORK=100
BLOCK_NUMBER=29707218

Utils refactor

The utils.ts was too big, i just broke it done into a few files.

Not included

  • I have to do a typescript cast because the type we persist in the model don't match what we actually store. We should change the model, but for that we need to add migrations. This is completely out of scope of this PR: refactor: model #18
  • Follow up PR will handle other results from the polling, like

@socket-security
Copy link

socket-security bot commented Aug 24, 2023

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
graphql-request 6.1.0 None +1 207 kB jasonkuhrt

🚮 Removed packages: @cowprotocol/[email protected]

@anxolin anxolin requested review from mfw78 and a team August 24, 2023 20:25
actions/poll.ts Outdated Show resolved Hide resolved
@anxolin
Copy link
Contributor Author

anxolin commented Aug 28, 2023

Documenting an issue im having:

If you add this env:

NETWORK=100
BLOCK_NUMBER=29490646

And run with yarn start:actions

You will reproduce this:

This awaited promised were returning the 3 strings we need:
image

Now they return undefined
image

Its like the params is now an array of string and not an object with the named props (as typescript thinks):
image

Reference https://github.com/cowprotocol/tenderly-watch-tower/blob/3be7215b063dfa52091e96d78fb440e4c87fee30/actions/checkForAndPlaceOrder.ts#L170

@anxolin anxolin changed the title Do polling and handle polling result Use SDK to poll and create smart order and handle result Aug 29, 2023
@anxolin anxolin marked this pull request as ready for review August 29, 2023 16:41
actions/checkForAndPlaceOrder.ts Outdated Show resolved Hide resolved
actions/checkForAndPlaceOrder.ts Outdated Show resolved Hide resolved
actions/checkForAndPlaceOrder.ts Outdated Show resolved Hide resolved
actions/test/run_local.ts Outdated Show resolved Hide resolved
actions/utils/context.ts Outdated Show resolved Hide resolved
actions/utils/misc.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@mfw78 mfw78 left a comment

Choose a reason for hiding this comment

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

An epic review. A few nits, and comments specifically dealing with merkle roots of orders.

actions/model.ts Show resolved Hide resolved
.gitignore Show resolved Hide resolved
actions/utils.ts Outdated Show resolved Hide resolved
actions/checkForAndPlaceOrder.ts Show resolved Hide resolved
actions/utils/misc.ts Show resolved Hide resolved
actions/utils/provider.ts Outdated Show resolved Hide resolved
actions/checkForAndPlaceOrder.ts Outdated Show resolved Hide resolved
actions/checkForAndPlaceOrder.ts Outdated Show resolved Hide resolved
actions/checkForAndPlaceOrder.ts Outdated Show resolved Hide resolved
actions/checkForAndPlaceOrder.ts Show resolved Hide resolved
@anxolin anxolin merged commit b05a187 into main Aug 30, 2023
1 check passed
@alfetopito alfetopito deleted the poll-conditional-orders branch August 31, 2023 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants