Skip to content

Conversation

@Brechtpd
Copy link
Contributor

#2334

Currently I have set the multiplier to 50, so now it's also possible to set the fee up to 31.5% in steps of 0.5%. But is easy to change if some other value would make more sense.

All inputs to the circuit remains, it's just that feeBips values > 0.63% will automatically enable the new mode so the passed in feeBips values need to be multiples of 0.5%.

@Brechtpd Brechtpd requested review from dong77 and letsgoustc April 14, 2021 02:15
@dong77 dong77 requested a review from yueawang April 14, 2021 08:00
orderA.fillAmountBorS.bits,
VariableArrayT(1, state.constants._0),
orderA.feeBips.bits,
orderA.feeBipsMultiplierFlag.bits,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just one question, does this flag open to client user?
A user can not control whether his feebips is .63% or 31.5% as it is not a part of the message signed by the user, is it??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The user still just signs a maxFeeBips, but that value can now take up 12 bits instead of 6 bits previously. It's up to the relayer to set the feeBips to a value <= maxFeeBips and to a valid value that can be encoded correctly with/without the multiplier (so one of the old values, but also now up to 3150 in steps of 50). The new mode is automatically triggered when feeBips >= 64:

if (toBigInt(order.feeBips) >= 64 /*2**NUM_BITS_BIPS_DA*/)
{
    feeBipsMultiplierFlag.generate_r1cs_witness(pb, ethsnarks::FieldT(1));
    feeBipsData.generate_r1cs_witness(
      pb, ethsnarks::FieldT((toBigInt(order.feeBips) / FEE_MULTIPLIER).to_int()));
}
else
{
    feeBipsMultiplierFlag.generate_r1cs_witness(pb, ethsnarks::FieldT(0));
    feeBipsData.generate_r1cs_witness(pb, order.feeBips);
}

If decoding this doesn't match feeBips than the circuit is not valid, so it needs to be one of the accepted values.

By doing things this way everything remains 100% backward compatible (including the order signature).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I know that, I mean as the real fee is actually controlled by us, we can take either 0.63% or 31.5% if a user signs his maxFeeBips is 63. So if I am a user reading the API, I would assume the maxFeeBips is 31.5% rather than 0.63%, as I have no way to set the maxFeeBips to exactly 0.63%.

Copy link
Contributor Author

@Brechtpd Brechtpd Apr 16, 2021

Choose a reason for hiding this comment

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

the real fee is actually controlled by us, we can take either 0.63% or 31.5% if a user signs his maxFeeBips is 63.

That's not the case, the user signs a simple normal value between 0 <= maxFeeBips < 2**12. If the user wants to set the fee to 0.63% he sets maxFeeBips = 63. If the user wants to set the fee to 2%, he sets maxFeeBips = 200. How this value is encoded internally does not matter.

I have no way to set the maxFeeBips to exactly 0.63%

All values set by the user are exact. We just can't use them all internally because of the way the actual fee value is encoded, but the actual fee value used in a trade still needs to be <= maxFeeBips, which is exactly the same behavior as before.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I thought the maxFeeBips is still 6 bits because of the word 100% back compatible. So here the input data (eth calldata?) is changed, is it??
Also, I was thinking about the data reconstruction tool we might have built for 3.1 ( not sure if we have it or not), and I thought it was a simple parser tool. Now in 3.6 can we still rebuild the MKL tree from the very beginning with a simple parser? I guess the tool needs to be frequently patched due to changes like this. BTW: We may not have such a requirement, just wondering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I thought the maxFeeBips is still 6 bits because of the word 100% back compatible. So here the input data (eth calldata?) is changed, is it??

maxFeeBips is never put on-chain, only the actual feeBips is.

Also, I was thinking about the data reconstruction tool we might have built for 3.1 ( not sure if we have it or not), and I thought it was a simple parser tool. Now in 3.6 can we still rebuild the MKL tree from the very beginning with a simple parser? I guess the tool needs to be frequently patched due to changes like this. BTW: We may not have such a requirement, just wondering.

The calldata is only changed if the new mode is used. That's because in 3.6 there was one unused bit that was always 0. With the new fee encoding that bit is still 0 when using the previous encoding. But when the new mode is used that bit is now 1. So yes, parsing tools need to be updated to be able to handle the new mode, but the same logic can be used for decoding all data for all blocks old and new.

@kongliangzhong kongliangzhong merged commit 23ffef0 into master Apr 20, 2021
@kongliangzhong kongliangzhong deleted the trading-fee branch April 20, 2021 08:18
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