Bonfida / dex-v4

Orderbook-based on-chain SPL token swap market
101 stars 32 forks source link

Collecting the accumulated royalties from the market tries to send more quote token amount than expected #98

Closed mihneacalugaru closed 1 year ago

mihneacalugaru commented 1 year ago

Hello,

I have created a market using the following:

Then I have set up 2 wallets: a seller (also the one and only creator with 100% share of the royalties fees) and a buyer. Then I have done the following:

  1. Seller sells 1 base token for 1 quote token (UI price)
  2. Buyer buys 1 base token for 1 quote token. He was prompted to actually send 1.089 quote tokens
  3. Buyer settles and gets 1 base token
  4. Seller consumes events, settles and gets the 0.99 base tokens
  5. Seller invokes sweep fees --> this transaction fails because it tries to send 990 quote tokens to the creator. It should have sent him around 0.099 - 0.1 quote tokens. It seems that it tries to send him 10000 times more then it actually should. Seems like this 10000 it's the same number as the quote multiplier, in case that might have something to do with it.

The transaction can be found at: https://solscan.io/tx/FUtC7fDkJuLTBkPuBmNXKutCNL3XYZJkTmZr176gPdcCv6z6UQQ3MhquUwRacAxQRQDefEgGEuPzUa9L3tske5x?cluster=devnet

I looked a bit through the code, but I don't find a place where a raw amount (with decimals)/UI amount (no decimals) conversion or scale/unscale conversion is missed.

Appreciate it a lot if you could help me with this!

ellttBen commented 1 year ago

HI @mihneacalugaru! Parsing your current DexState, the accumulated_fees field has a value of 99000, which represents 0.99 quote tokens. Furthermore, looking at the program's logic, there is no code to multiply this field by the quote_currency_multiplier. This means that we are unable to reproduce your problem, which indicates that the most likely cause is a wrong deployment. Did you build the program on the main branch and then deploy it? Did you modify the code in any way?

mihneacalugaru commented 1 year ago

Hi @ellttBen and thanks for answering!

No, I have not done any change, nor did I deploy it myself. I am using the devnet program with this ID: HEHEVQNZMTmjUt3CUKdddqHWa4dtXqA9QUsi7QptCCHj.

Is there any other latest deployment on devnet?

ellttBen commented 1 year ago

Ah, can you try this program id : FEYrapVp9zfXNin7SR31Wv6BmTYDQYzsyK4aSxWgtRgf? I just deployed it to be sure.

mihneacalugaru commented 1 year ago

@ellttBen, I surely can do that, but I think I need a bit of help because we use the @bonfida/dex-v4 NPM package.

Some of the exported functions in there allow us to specify what programId we want to use (bindings/createMarket for example) and others do not (openOrders/makeCreateAccountTransaction for example). The latter, for example, invokes initializeAccount with the default value for programId, which is hardcoded as HEHEVQNZMTmjUt3CUKdddqHWa4dtXqA9QUsi7QptCCHj in the ids.ts, so we cannot change it cleanly.

It would be so lovely if all the exported functions that return TransactionInstruction objects (or PrimedTransaction objects) would allow the callers to specify the DEX program ID they want to use.

Or, at least, if you could update the main branch's DEX_ID in ids.ts to point to the DEX program you just deployed at FEYrapVp9zfXNin7SR31Wv6BmTYDQYzsyK4aSxWgtRgf would be lovely as well.

Or, if you have any other idea, let me know. This is all what I could think of at the moment.

ellttBen commented 1 year ago

Would PR #99 be enough?

mihneacalugaru commented 1 year ago

@ellttBen Looks like it is enough, yes.

(because it seems like the other instructions are either using market.programId (which is perfectly fine because createMarket allows for a custom programId) or are allowing a custom programId parameter)

Just a suggestion: if you do it for makeCreateAccountTransaction , you can also do it for makeCloseAccountTransaction for consistency reasons.

mihneacalugaru commented 1 year ago

Also, @ellttBen /@dr497, will this come with a version bump of the @bonfida/dex-v4 NPM package?

It will be really helpful.

Thanks!

ellttBen commented 1 year ago

Hey @mihneacalugaru, just published version 0.3.4 to npm

mihneacalugaru commented 1 year ago

Thanks a lot, @ellttBen!

I think we missed something though. The initializeAccount function called in the makeCreateAccountTransaction from openOrders is missing the programId parameter. It was passed to makeCreateAccountTransaction, but not further to initializeAccount.

Would you mind helping with a change on that and also an NPM version bump? Sorry for not seeing it earlier and for the friction.

ellttBen commented 1 year ago

It's alright, just pushed 0.3.5 with that fix, thanks for the catch!

mihneacalugaru commented 1 year ago

Thank you, @ellttBen!

I have run npm i @bonfida/dex-v4 to update to 0.3.5. However, this seems to have introduced a problem with signing and sending the instruction returned by OpenOrders.makeCreateAccountTransaction without yet using the new programId parameter. - so without changing the previous code I had there, but just by updating the package to 0.3.5.

I was just trying to test if everything is still backwards okay before I use FEYrapVp9zfXNin7SR31Wv6BmTYDQYzsyK4aSxWgtRgf as the programId for the OpenOrders.makeCreateAccountTransaction and I got an issue while signing and sending the transaction with my WalletAdapter.

It says Error: Signature verification failed. It doesn't even make it to the program, the wallet signals that the signature verification failed. image

More precisely, when it tries to serialize the transaction:

image

I don't know if this happened due to the update of the js bindings because it won't make sense to do so I guess. I have seen that when I updated the @bonfida@dex-v4 package one of its dependencies, @bonfida/aaob also got updated to 0.1.5. I don't know if this may have anything to do with what I am experiencing right now.

Would reply appreciate your help!

mihneacalugaru commented 1 year ago

@ellttBen, I think I know what happened.

When you passed the programId to initializeAccount you might have forgot that the feePayer is being expected before the programId, meaning you passed the programId in the place of the feePayer.

image

You can set the feePayer there as the owner.

ellttBen commented 1 year ago

Thanks for the catch @mihneacalugaru , we're up to 0.3.6 now

mihneacalugaru commented 1 year ago

Thank you very much @ellttBen, that worked.

I went deeper and made methods from market.ts to accept a programId parameter in order to pass it further to downstream methods from openOrders that need it.

Here's the PR: https://github.com/Bonfida/dex-v4/pull/100

ellttBen commented 1 year ago

Thanks for the PR @mihneacalugaru , just merged and deployed 0.3.7.

mihneacalugaru commented 1 year ago

@ellttBen thank you very much for your support here, the royalties are working as expected from my tests.

This means that the DEX program ID HEHEVQNZMTmjUt3CUKdddqHWa4dtXqA9QUsi7QptCCHj which is referenced in this repo is the result of the deployment of an older version of the program code.

Your latest deployment at FEYrapVp9zfXNin7SR31Wv6BmTYDQYzsyK4aSxWgtRgf seems to be working as expected, in case you want to replace the DEX program ID in the repo with that one.

I will close this as solved, thanks again!

dr497 commented 1 year ago

Thanks @mihneacalugaru - the package has been updated in 0.3.8