Poll conditional orders + Allow custom logic to delay checks to a given date, block, or forever #149

anxolin · 2023-08-23T15:50:44Z

Implement some basic Polling
Also, applies some follow-ups from: comments in #147

Implement a Polling (as a Template Method in ComposableCow)

This generic polling logic will check the following:

1. Validate concrete order: The order might have wrong parameters (or malformed ones). This method verifies it. In case it fils it will describe what was the issue (returns a reason).
2. Poll validate: Let's concretely order apply some polling validation. It will return undefined if the polling checks should continue, otherwise it will instruct that is not time to create an order. For this, it can signal:
- 2.1 TRY_NEXT_BLOCK: This block is not good, but next one might
- 2.2 TRY_ON_BLOCK: This block is not good, but try again in the given blockNumber
- 2.3 TRY_AT_DATE: This block is not good, but try again after a given date
- 2.4 DONT_TRY_AGAIN: This block is not good, but also the following blocks. Please don't try again because it will keep not being the right time!
3. Is Authorised: Verify if the owner authorised the order
4. Lastly, CALL getTradeableOrderWithSignature: Last check is to call and obtain the order. Ideally, most of the errors should have been handled before calling this function, this way, we only do this call when we are almost sure it will succeed.

isAuthorised

Method in ComposableCow to check if the order is authorised.

Exposed as public method, but also convenient for the polling logic.

Removed validation from constructor

I thought it was best to not do a validation in the constructor of the order, so we can programmatically call to isValid

Also the method isValid doesn't throw any more, instead returns the reason. This reasons are convenient for other methods like poll to give a good description of why the order is not yet ready.

Now polling can instruct to never poll again that smart order, and return a good reason why:

Generics for ConditionalOrders

Changed back to T and P as suggested in #147

Not implemented in this PR (follow up coming)

Custom logic in Twap conditional order to postpone some checks for the future (instead of checking on each block)

I left some TODOs:

github-actions · 2023-08-23T15:51:00Z

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

_{You can retrigger this bot by commenting recheck in this Pull Request}

coveralls · 2023-08-23T15:52:15Z

coverage: 77.586% (-3.8%) from 81.422% when pulling e7f2ae0 on poll-conditional-orders into a4c398c on small-nit-suggestions.

mfw78

Looks good overall. A few nits. Specifically with reference to ConditionalOrder, there does seem some ability for SDK users to confuse staticInput and data.

While comments are left suggesting to have a sharp delineation between SDK user and the SDK's internals (ie. data passed by the user, everything within the SDK talking about staticInput), if there's a strong opinion to go this way, that's cool, but I'd suggest documenting this in a @remark JSdoc note in the class' constructor so that SDK maintainers can link between the SDK and the smart contract mentally.

mfw78 · 2023-08-23T16:38:42Z

src/composable/types/Twap.ts

+    return !o.n.isZero()
+      ? {
+          partSellAmount: o.sellAmount.div(o.n),
+          minPartLimit: o.buyAmount.div(o.n),
+        }
+      : {
+          partSellAmount: BigNumber.from(0),
+          minPartLimit: BigNumber.from(0),
+        }


I'm not sure about this. I don't think a TWAP should ever be able to be instantiated with invalid data, and it would be invalid if the condition o.n >= 2 isn't satisfied.

I guess it's a protection for division by zero error

yes, it is a protection from division by zero.

I feel like, to make this generic, it would be nice to allow to create an invalid Twap class, and then validate it and get the error you have.

This is why I prefer to not throw in the constructor, and let the SDK user decide.

This way, you can blindly create the order from the staticInput and make use of the methods of the class.
I can experiment later using this in WatchTower, and we can decide to move it back to the constructor if we see is a bad idea.

src/composable/types.ts

src/composable/types/Twap.spec.ts

src/composable/ConditionalOrder.ts

mfw78 · 2023-08-23T18:06:36Z

src/composable/types/Twap.ts

-    // Second, verify that the order params are logically valid
-    Twap.isValid(staticInput)
-
    // Third, construct the base class using transformed parameters
    super({ handler: TWAP_ADDRESS, salt, staticInput, hasOffChainInput })
-
-    // Finally, verify that the transformed data is ABI-encodable
-    if (!isValidAbi(TWAP_DATA_ABI, [this.staticInput])) throw new Error('InvalidData')


If we can instantiate a concrete order that isn't valid at all times, I'd suggest that in Multiplexer within the add function that validation takes place to avoid SDK users inserting invalid orders (ideally, these should be caught well before the merkle tree attempts to generate itself).

Great point!

Added the check, and a test for it (great catch):

src/composable/types/Twap.ts

shoom3301

Haven't tested, but code looks perfect!

alfetopito · 2023-08-24T09:36:25Z

Unit tests are not happy

src/composable/ConditionalOrder.ts

src/composable/Multiplexer.spec.ts

anxolin · 2023-08-24T11:43:54Z

src/composable/types/Twap.ts

 */
-export type TwapData = {
+export type TwapDataBase = {


Shared model between the struct and the data

src/composable/types/Twap.ts

alfetopito · 2023-08-24T14:12:11Z

@anxolin unit tests were still broken

mfw78

Excellent work! Some small nits on typos, and methodology for authorizing proofs.

mfw78 · 2023-08-24T13:50:18Z

src/composable/types.ts

+  UNEXPECTED_ERROR = 'UNEXPECTED_ERROR',
+  TRY_NEXT_BLOCK = 'TRY_NEXT_BLOCK',
+  TRY_ON_BLOCK = 'TRY_ON_BLOCK',
+  TRY_AT_EPOCH = 'TRY_AT_DATE',


Nit: consistency

mfw78 · 2023-08-24T13:51:47Z

src/composable/contracts.ts

+import { ComposableCoW, ComposableCoW__factory } from './generated'
+import { ComposableCoWInterface } from './generated/ComposableCoW'
+
+let iCcomposableCow: ComposableCoWInterface | undefined


nit: suspected typo for isComposableCow ?

src/composable/ConditionalOrder.ts

mfw78 · 2023-08-24T14:01:44Z

src/composable/ConditionalOrder.ts

+   * Poll a conditional order to see if it is tradeable.
+   *
+   * @param owner The owner of the conditional order.
+   * @param p The proof and parameters.


The proof isn't potentially passed in. Suggest that this be an optional parameter (array of strings).

mfw78 · 2023-08-24T14:04:12Z

src/composable/ConditionalOrder.ts

+      }
+
+      // Check if the owner authorised the order
+      const isAuthorized = await this.isAuthorized(owner, chain, provider)


At this point, the proof parameter would be passed as an optional parameter.

mfw78 · 2023-08-24T14:20:32Z

src/composable/types/Twap.ts

+      if (!(sellToken != constants.AddressZero && buyToken != constants.AddressZero)) return 'InvalidToken'
+      if (!sellAmount.gt(constants.Zero)) return 'InvalidSellAmount'
+      if (!buyAmount.gt(constants.Zero)) return 'InvalidMinBuyAmount'
+      if (startTime.startType === StartTimeValue.AT_EPOC) {


mfw78 · 2023-08-24T14:24:12Z

src/composable/types/Twap.ts

+
+  const startTime: StartTime = span.isZero()
+    ? { startType: StartTimeValue.AT_MINING_TIME }
+    : { startType: StartTimeValue.AT_EPOC, epoch: startEpoch }


src/composable/Multiplexer.spec.ts

mfw78 · 2023-08-24T14:29:40Z

src/composable/Multiplexer.spec.ts

+  test("Can't add invalid conditional orders", () => {
+    // Given an invalid order, don't allow to add it to the multiplexer
+    const m = new Multiplexer(SupportedChainId.GNOSIS_CHAIN)
+    const invalidTwap = Twap.fromData({ ...generateRandomTWAPData(), timeBetweenParts: BigNumber.from(-1) })
+    expect(() => m.add(invalidTwap)).toThrow('Invalid order: InvalidFrequency')
+  })
+


mfw78 · 2023-08-24T14:31:36Z

src/composable/types/Twap.spec.ts

+    expect(
+      Twap.fromData({
+        ...TWAP_PARAMS_TEST,
+        startTime: { startType: StartTimeValue.AT_EPOC, epoch: BigNumber.from(-1) },


anxolin changed the base branch from main to small-nit-suggestions August 23, 2023 15:51

anxolin changed the title ~~Poll conditional orders~~ Poll conditional orders + Allow custom logic to delay checks to a given date, block, or forever Aug 23, 2023

This was referenced Aug 23, 2023

feat: Allow specific order implementations to signal the next checkTime cowprotocol/watch-tower#8

Closed

feat: Add Specific validation from smart orders by delegating to BaseConditionalOrder instances cowprotocol/watch-tower#7

Closed

anxolin requested review from mfw78 and a team August 23, 2023 17:06

mfw78 approved these changes Aug 23, 2023

View reviewed changes

shoom3301 approved these changes Aug 24, 2023

View reviewed changes

anxolin commented Aug 24, 2023

View reviewed changes

src/composable/ConditionalOrder.ts Show resolved Hide resolved

anxolin commented Aug 24, 2023

View reviewed changes

src/composable/ConditionalOrder.ts Show resolved Hide resolved

anxolin commented Aug 24, 2023

View reviewed changes

src/composable/Multiplexer.spec.ts Show resolved Hide resolved

anxolin commented Aug 24, 2023

View reviewed changes

src/composable/types/Twap.ts Show resolved Hide resolved

anxolin marked this pull request as ready for review August 24, 2023 11:45

anxolin mentioned this pull request Aug 24, 2023

Give context for invalid AppData #150

Open

2 tasks

anxolin force-pushed the poll-conditional-orders branch from fa68c36 to 433615e Compare August 24, 2023 12:56

anxolin added 11 commits August 24, 2023 13:56

WIP delete later

81c057a

Create poll and modify validations

0e926a7

Change back generic types

4e2c96b

Add auth check

d0703cc

Apply small fixes and add jsdoc

01d89f6

Add try on block option

025b6b8

Add jsdoc

992d8d4

Add todos

a4ff4b1

Fix wrong type

d0520a6

Add missing ABI

2b42fc0

Delete unused type

69e0aad

anxolin added 6 commits August 24, 2023 13:56

Remodel TWAP for user friendlyness

2f8d824

Fix utils

5ef5f54

Update to agreed model

b4c5350

Check order validation in multiplexer

15060c6

Move asert to abstract class

6f1e4e4

Remame consts

9931e1e

anxolin force-pushed the poll-conditional-orders branch from 433615e to 9931e1e Compare August 24, 2023 12:56

anxolin changed the base branch from small-nit-suggestions to main August 24, 2023 12:57

anxolin merged commit 019865a into main Aug 24, 2023

github-actions bot locked and limited conversation to collaborators Aug 24, 2023

alfetopito deleted the poll-conditional-orders branch August 24, 2023 14:11

mfw78 reviewed Aug 24, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Poll conditional orders + Allow custom logic to delay checks to a given date, block, or forever #149

Poll conditional orders + Allow custom logic to delay checks to a given date, block, or forever #149

anxolin commented Aug 23, 2023 •

edited

Loading

github-actions bot commented Aug 23, 2023

coveralls commented Aug 23, 2023 •

edited

Loading

mfw78 left a comment •

edited

Loading

mfw78 Aug 23, 2023

shoom3301 Aug 24, 2023

anxolin Aug 24, 2023

mfw78 Aug 23, 2023

anxolin Aug 24, 2023

anxolin Aug 24, 2023

shoom3301 left a comment

alfetopito commented Aug 24, 2023

anxolin Aug 24, 2023

mfw78 Aug 24, 2023

alfetopito commented Aug 24, 2023

mfw78 left a comment

mfw78 Aug 24, 2023

mfw78 Aug 24, 2023

mfw78 Aug 24, 2023

mfw78 Aug 24, 2023

mfw78 Aug 24, 2023

mfw78 Aug 24, 2023

mfw78 Aug 24, 2023

mfw78 Aug 24, 2023

Poll conditional orders + Allow custom logic to delay checks to a given date, block, or forever #149

Poll conditional orders + Allow custom logic to delay checks to a given date, block, or forever #149

Conversation

anxolin commented Aug 23, 2023 • edited Loading

Implement a Polling (as a Template Method in ComposableCow)

isAuthorised

Removed validation from constructor

Generics for ConditionalOrders

Not implemented in this PR (follow up coming)

github-actions bot commented Aug 23, 2023

coveralls commented Aug 23, 2023 • edited Loading

mfw78 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoom3301 left a comment

Choose a reason for hiding this comment

alfetopito commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfetopito commented Aug 24, 2023

mfw78 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anxolin commented Aug 23, 2023 •

edited

Loading

coveralls commented Aug 23, 2023 •

edited

Loading

mfw78 left a comment •

edited

Loading