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

improve(sdk): add support for Multicall2 contract #3325

Merged

Conversation

amateima
Copy link
Contributor

@amateima amateima commented Aug 24, 2021

Motivation

https://app.clubhouse.io/uma-project/story/2128/add-multicall-2-support-to-multicall-sdk-library

Summary

  • add Multipart2 contract interface & types

Testing

Check a box to describe how you tested these changes and list the steps for reviewers to test.

  • Ran end-to-end test, running the code as in production
  • New unit tests created
  • Existing tests adequate, no new tests required
  • All existing tests pass
  • Untested

@amateima amateima requested a review from daywiss August 24, 2021 15:32
@amateima amateima self-assigned this Aug 24, 2021
@shortcut-integration
Copy link

This pull request has been linked to Clubhouse Story #2128: add multicall 2 support to multicall sdk library.

@amateima amateima marked this pull request as ready for review August 24, 2021 15:34
Copy link
Contributor

@daywiss daywiss left a comment

Choose a reason for hiding this comment

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

sorry for the misunderstanding this morning, but yea i think it makes a lot of sense to separate m1 and m2 into separate folders. In my mind i had tasked this thinking you should also update the helper multicall class in lib which is what i was thinking of, but we can do that as a separate task.

anyways for the most part this looks good, but imo id say refactor it into separate clients. lmk what you think

packages/sdk/src/clients/multicall/client.ts Outdated Show resolved Hide resolved
packages/sdk/src/clients/multicall/client.e2e.ts Outdated Show resolved Hide resolved
packages/sdk/src/clients/multicall/client.e2e.ts Outdated Show resolved Hide resolved
@amateima amateima force-pushed the feature/ch2128/add-multicall-2-support-to-multicall-sdk branch from 95ee409 to f2ce3f0 Compare August 24, 2021 19:01
@amateima amateima requested review from daywiss and mrice32 August 25, 2021 13:38
Copy link
Contributor

@daywiss daywiss left a comment

Choose a reason for hiding this comment

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

This looks great, just one comment on the new test

packages/sdk/src/clients/multicall2/index.ts Show resolved Hide resolved
packages/sdk/src/clients/multicall2/client.e2e.ts Outdated Show resolved Hide resolved
@amateima amateima force-pushed the feature/ch2128/add-multicall-2-support-to-multicall-sdk branch from 09152b1 to f922f2a Compare August 25, 2021 15:06
@nicholaspai nicholaspai requested a review from chrismaree August 25, 2021 15:10
Copy link
Member

@nicholaspai nicholaspai left a comment

Choose a reason for hiding this comment

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

Great first PR, I did one pass and mainly left comments to add more comments to the smart contracts. On second pass after you re-request my review, I'll examine the sdk code more closely

Signed-off-by: amateima <amatei@umaproject.org>
Signed-off-by: amateima <amatei@umaproject.org>
Signed-off-by: amateima <amatei@umaproject.org>
Signed-off-by: amateima <amatei@umaproject.org>
@amateima amateima force-pushed the feature/ch2128/add-multicall-2-support-to-multicall-sdk branch from f922f2a to ec61c0c Compare August 25, 2021 15:13
@nicholaspai nicholaspai changed the title improve(sdk): add support for Multipart2 contract improve(sdk): add support for Multicall2 contract Aug 25, 2021
Signed-off-by: amateima <amatei@umaproject.org>
Copy link
Member

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

Looks awesome! This is really really nice code, and very consistent with the surrounding code and style of the repo. Very nice work @amateima!!

Just added a few minor nits and comments.

packages/core/contracts/common/interfaces/Multicall2.sol Outdated Show resolved Hide resolved
packages/sdk/src/clients/multicall2/client.e2e.ts Outdated Show resolved Hide resolved
packages/sdk/src/clients/multicall2/README.md Outdated Show resolved Hide resolved
packages/sdk/src/clients/multicall2/client.e2e.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@daywiss daywiss left a comment

Choose a reason for hiding this comment

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

Looks great, going to approve assuming you will address rest of comments! Try to get a second approval before merging and you should be good to go

Signed-off-by: amateima <amatei@umaproject.org>
Signed-off-by: amateima <amatei@umaproject.org>
Signed-off-by: amateima <amatei@umaproject.org>
@amateima amateima requested review from mrice32 and chrismaree August 26, 2021 23:45
Copy link
Member

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

LGTM!

@amateima amateima merged commit bb31e8f into master Aug 27, 2021
@amateima amateima deleted the feature/ch2128/add-multicall-2-support-to-multicall-sdk branch September 1, 2021 13:36
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.

5 participants