Small nit suggestions #147

anxolin · 2023-08-22T18:17:57Z

This is a misc on some NIT suggestions.

Happy to take any of this back, but I feel is still early (we are still doing a RC), and we can arrange things a bit.

I tried to explain the changes in the code, mainly

Rename things around: like BaseConditionalOrder is now ConditionalOrder, and TWAP is Twap
Created a utils.ts with helpers
Created a types.ts with main types
Moved some static methods from the abstract class to the utils.ts
Changed the name of the twap orders type to twap to match what we store onchain in orderClass for the appData

To review

I left some comments in the code too

github-actions · 2023-08-22T18:18:20Z

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-22T18:18:54Z

coverage: 81.422% (-0.1%) from 81.541% when pulling a4c398c on small-nit-suggestions into 0aad903 on main.

src/composable/ConditionalOrder.spec.ts

anxolin · 2023-08-22T18:20:24Z

src/composable/ConditionalOrder.spec.ts

@@ -68,21 +66,25 @@ describe('ConditionalOrder', () => {
      '0x910d00a310f7Dc5B29FE73458F47f519be547D3d',
      '0x9379a0bf532ff9a66ffde940f94b1a025d6f18803054c1aef52dc94b15255bbe'
    )
-    expect(BaseConditionalOrder.leafToId(order.leaf)).toEqual(
+    expect(ConditionalOrder.leafToId(order.leaf)).toEqual(


Just to keep things simple, i suggest we just call the abstract class ConditionalOrder instead of BaseConditionalOrder

There needs to be some room left for further generalisation.

All conditional orders that are done now are generators - but these are a subset of conditional orders. There may be conditional order types that only specify a verify, and these require custom logic to know what to provide them for offChain input etc.

I'm not sure where this logic would be implemented idiomatically.

src/composable/ConditionalOrder.spec.ts

anxolin · 2023-08-22T18:22:49Z

src/composable/ConditionalOrder.ts

-    hasOffChainInput = false
-  ) {
+  constructor(params: ConditionalOrderArguments<Params>) {
+    const { handler, salt = utils.keccak256(utils.randomBytes(32)), staticInput, hasOffChainInput = false } = params


I believe is better to use name parameters than a list of parameters, especially with optional ones.
This reduces errors using it and make params more explicit and easy to read

src/composable/ConditionalOrder.ts

src/composable/Multiplexer.ts

src/composable/types.ts

src/composable/types/Twap.ts

src/composable/utils.spec.ts

src/composable/utils.ts

src/composable/types/Twap.ts

mfw78

Looks great, thanks for the detailed, comprehensive review.

Mostly minor nits, but the one that stands out the most is the name of the generic used on ConditionalOrder. My fear in that using the generic Params may cause the SDK user to conflate this with the IConditionalOrder.ConditionalOrderParams struct.

mfw78 · 2023-08-23T10:17:18Z

src/composable/ConditionalOrder.spec.ts

@@ -68,21 +66,25 @@ describe('ConditionalOrder', () => {
      '0x910d00a310f7Dc5B29FE73458F47f519be547D3d',
      '0x9379a0bf532ff9a66ffde940f94b1a025d6f18803054c1aef52dc94b15255bbe'
    )
-    expect(BaseConditionalOrder.leafToId(order.leaf)).toEqual(
+    expect(ConditionalOrder.leafToId(order.leaf)).toEqual(


There needs to be some room left for further generalisation.

All conditional orders that are done now are generators - but these are a subset of conditional orders. There may be conditional order types that only specify a verify, and these require custom logic to know what to provide them for offChain input etc.

I'm not sure where this logic would be implemented idiomatically.

src/composable/utils.ts

src/composable/types.ts

src/composable/types/Twap.ts

src/composable/Multiplexer.spec.ts

src/composable/ConditionalOrder.ts

Co-authored-by: mfw78 <[email protected]>

anxolin · 2023-08-23T14:15:53Z

Nice comments!

I will address!

alfetopito · 2023-08-24T14:13:14Z

@anxolin unit tests also broken here

anxolin added 9 commits August 22, 2023 11:18

Rename file

88b7483

Move types and add some nitpics

106bf49

Add smal nitpics for TWAP

cd006d3

Add small nitpicks

8226be2

Change file names

6675141

Change file names back to original

71bca69

Renamings and moving static utils

03e17ca

Add encode decode params utils

23de41a

Add isValidAbi tests

ba7c0d2

anxolin marked this pull request as ready for review August 22, 2023 18:32

anxolin requested review from mfw78 and a team August 23, 2023 10:02

anxolin commented Aug 23, 2023

View reviewed changes

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

anxolin commented Aug 23, 2023

View reviewed changes

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

mfw78 approved these changes Aug 23, 2023

View reviewed changes

Change jsdoc

a4c398c

Co-authored-by: mfw78 <[email protected]>

mfw78 mentioned this pull request Aug 23, 2023

refactor: generalise handler checks #148

Open

anxolin mentioned this pull request Aug 23, 2023

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

Merged

anxolin merged commit f878bdd into main Aug 24, 2023

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

alfetopito deleted the small-nit-suggestions branch August 24, 2023 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small nit suggestions #147

Small nit suggestions #147

anxolin commented Aug 22, 2023 •

edited

Loading

github-actions bot commented Aug 22, 2023

coveralls commented Aug 22, 2023 •

edited

Loading

anxolin Aug 22, 2023

mfw78 Aug 23, 2023

anxolin Aug 22, 2023

mfw78 left a comment

mfw78 Aug 23, 2023

anxolin commented Aug 23, 2023

alfetopito commented Aug 24, 2023

Small nit suggestions #147

Small nit suggestions #147

Conversation

anxolin commented Aug 22, 2023 • edited Loading

To review

github-actions bot commented Aug 22, 2023

coveralls commented Aug 22, 2023 • edited Loading

anxolin Aug 22, 2023

Choose a reason for hiding this comment

mfw78 Aug 23, 2023

Choose a reason for hiding this comment

anxolin Aug 22, 2023

Choose a reason for hiding this comment

mfw78 left a comment

Choose a reason for hiding this comment

mfw78 Aug 23, 2023

Choose a reason for hiding this comment

anxolin commented Aug 23, 2023

alfetopito commented Aug 24, 2023

anxolin commented Aug 22, 2023 •

edited

Loading

coveralls commented Aug 22, 2023 •

edited

Loading