metaplex-foundation / js-deprecated

Deprecated Metaplex JavaScript SDK
https://metaplex-foundation.github.io/js/
MIT License
128 stars 77 forks source link

Error in actions.placeBid #155

Open dtome123 opened 2 years ago

dtome123 commented 2 years ago

There a bug when I call actions.placeBid function, it is "Bidder Pot Token Must be a new account". Then I read source code and everything is normal. Why did that happen? image (error) image (code)

exromany commented 2 years ago

since this https://github.com/metaplex-foundation/metaplex-program-library/pull/101

  const bidderPotToken = await AuctionProgram.findProgramAddress([
    Buffer.from(AuctionProgram.PREFIX),
    new PublicKey(bidderPotKey).toBuffer(),
    Buffer.from("bidder_pot_token"),
  ]);
aheckmann commented 2 years ago

Need to port this fix over https://github.com/metaplex-foundation/metaplex/pull/1557

dtome123 commented 2 years ago

I try like sugget of exromany then it pass old bug but it have new bug image

exromany commented 2 years ago

because simulation fails. you can use connection.sendRawTransaction with skipPreflight to skip simulation

connection.sendRawTransaction(tx.serialize(), {
  skipPreflight: true,
});
dtome123 commented 2 years ago

because simulation fails. you can use connection.sendRawTransaction with skipPreflight to skip simulation

connection.sendRawTransaction(tx.serialize(), {
  skipPreflight: true,
});

But after executing, it is still an error transaction so it's not better than. Do you think error from logic code somewhere like smart contract? Because if I use old code in JDK, error is "Bidder Pot Token must be a new account". Then I use your code, it pass but new error is "instruction requires an initialized account". Is there a conflict here? Thank for your help

basvanberckel commented 2 years ago

The issue is that the revoke instruction is sent after the accountclose instruction, which leads to an uninitializedaccount error.

AmmarKhalid123 commented 2 years ago

@basvanberckel @dtome123 I am facing this same issue, any idea on how to fix this ?

basvanberckel commented 2 years ago

I believe this is already enough to fix this error: https://github.com/metaplex-foundation/js/pull/172

AmmarKhalid123 commented 2 years ago

Yes this seems correct. I hope it gets accepted soon enough

AmmarKhalid123 commented 2 years ago

@basvanberckel Is there any way I can use your forked repo to test this solution, and continue on my work until your PR is committed? I have run npm install git+https://github.com/Beemup/metaplex-js-sdk.git but unable to import from @metaplex/js, maybe because "lib" folder is missing in the node_modules/@metaplex/js/, which was there when I installed usingnpm install @metaplex/js.

basvanberckel commented 2 years ago

What does your import/require statement look like in your app code? You shouldn't need to import from /lib, just from '@metaplex/js'

AmmarKhalid123 commented 2 years ago

import { Connection, actions } from '@metaplex/js';

AmmarKhalid123 commented 2 years ago

import { Connection, actions } from '@metaplex/js';

On this import, I'm getting error: Module not found: Can't resolve '@metaplex/js' in 'D:\Office\SolMarketplace\solmarket\src' . Am I doing something wrong? @basvanberckel

basvanberckel commented 2 years ago

Yeah, my bad, you can't directly install from this repo as the artifacts aren't included. Clone the repo, build it and install that as a dependency to your own project:

cd your/project/dir
git clone https://github.com/beemup/metaplex-js-sdk -b placebid-transfer-bug
cd metaplex-js-sdk
npm install
npm run build
cd ..
npm install ./metaplex-js-sdk
AmmarKhalid123 commented 2 years ago

@basvanberckel I'm still getting the same error btw :( Have you tested out this solution on your end?

AmmarKhalid123 commented 2 years ago

@basvanberckel I'm still getting the same error btw :( Have you tested out this solution on your end?

Just to clarify, imports are working fine, but I'm getting the initial error of "Bidder pot token must be a new account" on actions.placeBid @basvanberckel

basvanberckel commented 2 years ago

I have my own implementation of placebid that fixes both these errors, the fix linked in this thread above together with my pull request should resolve both. I am not able to write a full test for this right now, there should be a unit test if this needs to be verified in the future.

AmmarKhalid123 commented 2 years ago

the fix linked in this thread above together with my pull request should resolve both.

Are you talking about this fix?

because simulation fails. you can use connection.sendRawTransaction with skipPreflight to skip simulation

connection.sendRawTransaction(tx.serialize(), {
  skipPreflight: true,
});
basvanberckel commented 2 years ago

The bidder token address should be generated like in https://github.com/metaplex-foundation/metaplex/pull/1557

That will resolve the initial error of this issue.

The transaction order should be changed like in my PR, that will resolve the UninitializedAccount error. It is also an option to remove the revoke instruction entirely, like in https://github.com/metaplex-foundation/metaplex/pull/1557

basvanberckel commented 2 years ago

I've updated the pr to fix both errors. @AmmarKhalid123 Would you be able to verify this works in your setup?

AmmarKhalid123 commented 2 years ago

I've updated the pr to fix both errors. @AmmarKhalid123 Would you be able to verify this works in your setup?

I will check right away and let you know

AmmarKhalid123 commented 2 years ago

I've updated the pr to fix both errors. @AmmarKhalid123 Would you be able to verify this works in your setup?

Works like a charm.

AmmarKhalid123 commented 2 years ago

@basvanberckel forgot, there was an import of AuctionProgram I remember missing, placeBid.ts file. I got it when tried to build the project, otherwise works great.

basvanberckel commented 2 years ago

Ah yes, thanks. Fixed that now

tengu-br commented 2 years ago

Can also confirm that @basvanberckel fix works.

nurav97 commented 2 years ago

@AmmarKhalid123 @basvanberckel @tengu-br i am also facing the same problem of bidder pot, i made the changes you have added in the pr but the issue of bidder pot token must be new is still there am i missing something