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

feat(uniswapx-sdk): Update Structs & ABI functionality for Dutch V3 Gas Adjustment Feature #106

Open
wants to merge 5 commits into
base: v3-dutch-sdk
Choose a base branch
from

Conversation

alanhwu
Copy link

@alanhwu alanhwu commented Sep 23, 2024

Description

Structural changes for DutchV3 to support new gas adjustment feature

This unblocks us to now write E2E integration tests

How Has This Been Tested?

Tests have been updated to check and use this field. There is no formal checking of logical bounds of the feature, which should be done when we implement support for this feature to be used.

@alanhwu alanhwu requested a review from a team as a code owner September 23, 2024 21:23
Copy link

graphite-app bot commented Sep 23, 2024

Graphite Automations

"Request reviewers once CI passes on sdks monorepo" took an action on this PR • (09/23/24)

1 reviewer was added and 1 assignee was added to this PR based on 's automation.

@@ -364,13 +406,51 @@ describe("V3DutchOrderBuilder", () => {
).toThrow("Invariant failed: nonce not set");
});

it("Throw if startingBaseFee not set", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a test for when startingBaseFee and adjustmentPerGweiBaseFee are non-zero (e.g. 1e9)

Copy link
Contributor

@codyborn codyborn left a comment

Choose a reason for hiding this comment

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

Looks good! Left a minor comment.

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