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

Fix: FQT otc order encoding logic [TKR-611] #596

Merged
merged 5 commits into from
Oct 12, 2022

Conversation

dextracker
Copy link
Contributor

Description

swap rfq and otc in fillQuoteTransformerDataEncoder to match our enum

Testing instructions

Types of changes

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • Add new entries to the relevant CHANGELOG.jsons.

@dextracker dextracker requested a review from dekz as a code owner October 7, 2022 14:44
@dextracker dextracker requested review from abls and kyu-c October 7, 2022 14:44
Copy link
Contributor

@abls abls left a comment

Choose a reason for hiding this comment

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

I don't believe this will fix it. You have to remove the otcOrders field completely, it isn't present on-chain at all.

IMO would be best to just revert this commit.

@kyu-c
Copy link
Contributor

kyu-c commented Oct 7, 2022

Another approach is keeping two versions of FQT encoders (legacy, and otc-supporting version).
On the 0x-api maybe introduce a feature flag to determine which encoder to use.
Once the new FQT is deployed to all chains then we can do the cleanup (remove flag, retire the legacy FQT encoder)

@@ -87,6 +82,11 @@ export const fillQuoteTransformerDataEncoder = AbiEncoder.create([
{ name: 'fillSequence', type: 'uint8[]' },
{ name: 'fillAmount', type: 'uint256' },
{ name: 'refundReceiver', type: 'address' },
{
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't there be an accompanying change in FQT (solidity code)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup forgot to add from /protocol was in protocol-utils

Copy link
Contributor

@kyu-c kyu-c left a comment

Choose a reason for hiding this comment

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

LGTM

@dextracker dextracker merged commit bf1b5c4 into development Oct 12, 2022
@dextracker dextracker deleted the fix/FqtOtcEncoding branch October 12, 2022 21:10
@dextracker dextracker changed the title Fix: FQT otc order encoding logic Fix: FQT otc order encoding logic [TKR-611] Oct 13, 2022
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