Skip to content

Conversation

cece-z
Copy link
Contributor

@cece-z cece-z commented Sep 20, 2021

Code Review Processes

New Feature Review

Before submitting a pull request for new review, make sure the following is done:

  • Design doc is created and posted here: [Insert Link]
  • Code cleanliness and completeness is addressed via guidelines

README Checks

  • README has proper context for the reviewer to understand what the code includes, any important design considerations, and areas to pay more attention to

Code Checks

  • Add explanatory comments. If there is complex code that requires specific context or understanding, note that in a comment
  • Remove unncessary comments. Any comments that do not add additional context, information, etc. should be removed
  • [] Add javadocs.
  • Scrub through the code for inconsistencies (e.g. removing extra spaces)
  • Ensure there are not any .onlys in spec files

Broader Considerations

  • Ensure variable, function and event naming is clear, consistent, and reflective for the scope of the code.
  • Consider if certain pieces of logic should be placed in a different library, module

@cece-z cece-z changed the title Update ZeroExApiAdapter to support multiplex [WIP] Update ZeroExApiAdapter to support multiplex Sep 23, 2021
@cece-z cece-z marked this pull request as ready for review September 24, 2021 17:40
@cece-z cece-z changed the title [WIP] Update ZeroExApiAdapter to support multiplex Update ZeroExApiAdapter to support multiplex Sep 24, 2021
Copy link

@moodlezoup moodlezoup left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@bweick
Copy link
Contributor

bweick commented Sep 27, 2021

Let's remove the ETH related functions...we don't want anyone dealing directly with ETH since our system doesn't support it.

@bweick
Copy link
Contributor

bweick commented Sep 27, 2021

Also if you wouldn't mind bumping the package version to 0.1.1 that would be great so we can export these changes.

@cece-z
Copy link
Contributor Author

cece-z commented Sep 27, 2021

@bweick Thanks. Should I also remove two old functions sellTokenForEthToUniswapV3() and sellEthForTokenToUniswapV3() as well?

@bweick
Copy link
Contributor

bweick commented Sep 27, 2021

@bweick Thanks. Should I also remove two old functions sellTokenForEthToUniswapV3() and sellEthForTokenToUniswapV3() as well?

yes let's

@bweick bweick merged commit 22a77d5 into SetProtocol:master Sep 27, 2021
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.

4 participants