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 enum parsing bug #2763

Merged
merged 7 commits into from
Jan 9, 2024
Merged

Conversation

0xabhinav
Copy link
Contributor

No description provided.

Copy link

vercel bot commented Jan 4, 2024

@0xabhinav is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor Author

@0xabhinav 0xabhinav left a comment

Choose a reason for hiding this comment

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

Thanks for building this library! Saw some bugs while trying to self-parse transactions and wanted to fix back upstream :)

One more potential bug:-
We make map against camelCase name in parseTxLayout, perhaps the constructor of BorshInstructionCoder also needs to change to:-

public constructor(private idl: Idl) {
    this.ixLayout = BorshInstructionCoder.parseIxLayout(idl);
    const sighashLayouts = new Map();
    idl.instructions.forEach((ix) => {
      const sh = sighash(SIGHASH_GLOBAL_NAMESPACE, ix.name);
      sighashLayouts.set(bs58.encode(sh), {
        // The line below was changed
        layout: this.ixLayout.get(camelCase(ix.name)),
        name: ix.name,
      });
    });
    this.sighashLayouts = sighashLayouts;
  }

ts/packages/anchor/src/coder/borsh/instruction.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this problem!

We make map against camelCase name in parseTxLayout, perhaps the constructor of BorshInstructionCoder also needs to change to:-

layout: this.ixLayout.get(camelCase(ix.name))

Instructions are already camelCased in the IDL so it's not clear to me what this fixes.

There are also a few CI errors: https://github.com/coral-xyz/anchor/actions/runs/7410828061/job/20183971928?pr=2763

ts/packages/anchor/src/coder/borsh/instruction.ts Outdated Show resolved Hide resolved
ts/packages/anchor/src/coder/borsh/instruction.ts Outdated Show resolved Hide resolved
@0xabhinav
Copy link
Contributor Author

Thank you for fixing this problem!

We make map against camelCase name in parseTxLayout, perhaps the constructor of BorshInstructionCoder also needs to change to:-

layout: this.ixLayout.get(camelCase(ix.name))

Instructions are already camelCased in the IDL so it's not clear to me what this fixes.

There are also a few CI errors: https://github.com/coral-xyz/anchor/actions/runs/7410828061/job/20183971928?pr=2763

Yeah that's why I didn't make this change. Don't have enough context of anchor's idl generation logic :) Is there some doc you recommend for this?

Copy link
Contributor Author

@0xabhinav 0xabhinav left a comment

Choose a reason for hiding this comment

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

Tests should be fixed now, can you take a look again pls?

PS: We're building a social explorer @ 0xppl.com and I'm currently working on bringing all our EVM multichain features to the solana ecosystem :) Happy to extend a beta invite to you! This PR helps us improve our parsing quality of sol txns

ts/packages/anchor/src/coder/borsh/instruction.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

Choose a reason for hiding this comment

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

Tests should be fixed now, can you take a look again pls?

Thanks, I've tried this with your example IDL and the problem seems to be caused by the enum variant name being PascalCase in the IDL but camelCase as input, so this part was enough to fix it.

PS: We're building a social explorer @ 0xppl.com and I'm currently working on bringing all our EVM multichain features to the solana ecosystem :) Happy to extend a beta invite to you! This PR helps us improve our parsing quality of sol txns

That's great to hear and thank you, I can take a look but unfortunately I don't have much time to test stuff these days.

@acheroncrypto acheroncrypto merged commit 211982a into coral-xyz:master Jan 9, 2024
48 of 49 checks passed
@0xabhinav 0xabhinav deleted the bug/nestedenumparsing branch January 10, 2024 09:17
@0xabhinav
Copy link
Contributor Author

Thanks! Is it possible for me to refer to this build in my package.json somehow?

@acheroncrypto
Copy link
Collaborator

You can add it as a local dependency if your package is not being published, e.g.

yarn add file:../anchor

otherwise this change will be available in the next release.

@vserpokryl
Copy link

@acheroncrypto How about publish a new version of @coral-xyz/anchor. We need these changes

@acheroncrypto
Copy link
Collaborator

I'll publish after #2824 is merged, which changes how parsing works and fixes more problems similar to this PR.

Also, creating your own parser based on your needs would be a good idea because the next release won't support old IDLs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants