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

JS: Constant SC_SPEC_DOC_LIMIT doesn't get replaced with a JavaScript XDR constant #152

Open
wildework opened this issue Feb 16, 2023 · 12 comments
Labels

Comments

@wildework
Copy link

wildework commented Feb 16, 2023

What version are you using?

I'm using the 6e42b01e1ceb25c251d4c4d45443d7e0f228822a commit version.

What did you do?

From the root folder of the repository, I call xdrgen.

./bin/xdrgen -o ../stellar_sdk/xdr -l javascript -n stellar ../stellar-xdr/*.x

Every instance of SC_SPEC_DOC_LIMIT is not replaced with a constant lookup, here's one example.

struct SCSpecUDTStructFieldV0
{
    string doc<SC_SPEC_DOC_LIMIT>;
    string name<30>;
    SCSpecTypeDef type;
};

What did you expect to see?

xdr.struct("ScSpecUdtStructFieldV0", [
  ["doc", xdr.string(xdr.lookup("SC_SPEC_DOC_LIMIT"))],
  ["name", xdr.string(30)],
  ["type", xdr.lookup("ScSpecTypeDef")],
]);

What did you see instead?

xdr.struct("ScSpecUdtStructFieldV0", [
  ["doc", xdr.string(SC_SPEC_DOC_LIMIT)],
  ["name", xdr.string(30)],
  ["type", xdr.lookup("ScSpecTypeDef")],
]);
@wildework wildework added the bug label Feb 16, 2023
@wildework
Copy link
Author

Also, the same issue appears withSCVAL_LIMIT in this instance.

union SCObject switch (SCObjectType type)
{
case SCO_VEC:
    SCVec vec;
case SCO_MAP:
    SCMap map;
case SCO_U64:
    uint64 u64;
case SCO_I64:
    int64 i64;
case SCO_U128:
    Int128Parts u128;
case SCO_I128:
    Int128Parts i128;
case SCO_BYTES:
    opaque bin<SCVAL_LIMIT>;
case SCO_CONTRACT_CODE:
    SCContractCode contractCode;
case SCO_ADDRESS:
    SCAddress address;
case SCO_NONCE_KEY:
    SCAddress nonceAddress;
};

Where I get the JavaScript generated without xdr.lookup.

xdr.union("ScObject", {
  switchOn: xdr.lookup("ScObjectType"),
  switchName: "type",
  switches: [
    ["scoVec", "vec"],
    ["scoMap", "map"],
    ["scoU64", "u64"],
    ["scoI64", "i64"],
    ["scoU128", "u128"],
    ["scoI128", "i128"],
    ["scoBytes", "bin"],
    ["scoContractCode", "contractCode"],
    ["scoAddress", "address"],
    ["scoNonceKey", "nonceAddress"],
  ],
  arms: {
    vec: xdr.lookup("ScVec"),
    map: xdr.lookup("ScMap"),
    u64: xdr.lookup("Uint64"),
    i64: xdr.lookup("Int64"),
    u128: xdr.lookup("Int128Parts"),
    i128: xdr.lookup("Int128Parts"),
    bin: xdr.varOpaque(SCVAL_LIMIT),
    contractCode: xdr.lookup("ScContractCode"),
    address: xdr.lookup("ScAddress"),
    nonceAddress: xdr.lookup("ScAddress"),
  },
});

This line in particular.

bin: xdr.varOpaque(SCVAL_LIMIT),

Here's another example.

struct ContractCodeEntry {
    ExtensionPoint ext;

    Hash hash;
    opaque code<SCVAL_LIMIT>;
};

I get no xdr.lookup.

xdr.struct("ContractCodeEntry", [
  ["ext", xdr.lookup("ExtensionPoint")],
  ["hash", xdr.lookup("Hash")],
  ["code", xdr.varOpaque(SCVAL_LIMIT)],
]);

@leighmcculloch
Copy link
Member

@paulbellamy Did you encounter any issues with constants when generating the code for js-stellar-base? We've had the SCVAL_LIMIT constant in the XDR for months at this point, and it looks like we generated the code just fine in https://github.com/stellar/js-stellar-base/blob/master/src/generated/next_generated.js.

@leighmcculloch
Copy link
Member

What did you expect to see?

xdr.struct("ScSpecUdtStructFieldV0", [
  ["doc", xdr.string(SC_SPEC_DOC_LIMIT)],
  ["name", xdr.string(30)],
  ["type", xdr.lookup("ScSpecTypeDef")],
]);

What did you see instead?

xdr.struct("ScSpecUdtStructFieldV0", [
  ["doc", xdr.string(xdr.lookup("SC_SPEC_DOC_LIMIT"))],
  ["name", xdr.string(30)],
  ["type", xdr.lookup("ScSpecTypeDef")],
]);

@wildework Are you asking that every location should use lookup, or should not use lookup? I just want to confirm that because the issue description says the lookup shouldn't be there, but the following comment with examples sounds like the lookup should be there.

@wildework
Copy link
Author

@leighmcculloch most instances of SCVAL_LIMIT are fine, it's just the ones I shared that don't work, like in this example.

xdr.varOpaque(SCVAL_LIMIT)

As a constant SCVAL_LIMIT is never defined, nor is any other constant. js-xdr is only retrieving constants using xdr.lookup method.

@wildework
Copy link
Author

wildework commented Feb 16, 2023

@leighmcculloch And sorry about the mix-up, I flipped the order for "expect" and "did see". I have fixed my original post with the correct order now. And just to be explicit - we need to use xdr.lookup(), as per my comment above.

@leighmcculloch leighmcculloch changed the title Constant SC_SPEC_DOC_LIMIT doesn't get replaced with a JavaScript XDR constant JS: Constant SC_SPEC_DOC_LIMIT doesn't get replaced with a JavaScript XDR constant Feb 17, 2023
@ire-and-curses
Copy link
Member

@wildework What are the direct consequences for you of this bug? Trying to understand the impact and therefore the urgency.

@wildework
Copy link
Author

@ire-and-curses For me, it's a matter of manually editing the generated JS code.

I've already solved this downstream in my sdk https://github.com/useSoroban/sdk/blob/trunk/source/xdr.js#L748 so this bug only affects people that will be using the official Stellar JavaScript SDK and trying to parse data of types that rely on SC_SPEC_DOC_LIMIT.

@Shaptic
Copy link
Contributor

Shaptic commented Feb 17, 2023

it looks like we generated the code just fine in https://github.com/stellar/js-stellar-base/blob/master/src/generated/next_generated.js

not quite, @leighmcculloch: https://github.com/stellar/js-stellar-base/blob/f508a02f239df9810a35e3704d2081dbdf05a3c9/src/generated/next_generated.js#L7931-L7952

It seems to fail in the case of unions and structs that use the constant to define an array's size, e.g. struct Name { type name<CONSTANT> }; won't do an xdr.lookup("CONSTANT").

@Shaptic
Copy link
Contributor

Shaptic commented Apr 20, 2023

Basically, there are no downstream risks because we have to fix this manually anyway to release new versions of stellar-base@soroban (it won't build otherwise). However, it's still an annoying amount of manual work that'd be great to fix.

(Note that this wasn't done for stellar-base@latest as noted by Leigh above in #152 (comment) because the vNext XDR isn't used, only generated. This is an oversight, admittedly, but it harms nobody because they can't access it.)

@mollykarcher mollykarcher moved this from Next Sprint Proposal to Backlog in Platform Scrum May 16, 2023
@wildework
Copy link
Author

wildework commented Sep 10, 2023

Posting to keep this alive. I pulled the latest version of xdrgen as of 9/10/23 and the JavaScript output stellar_generated.js still produces this error Uncaught ReferenceError: SC_SPEC_DOC_LIMIT is not defined.

The same with SCSYMBOL_LIMIT.

@Shaptic
Copy link
Contributor

Shaptic commented Sep 11, 2023

Yep, unfortunately we're still doing the manual workaround as this bug hasn't been a priority: https://github.com/stellar/js-stellar-base/blob/72887306042382f8faffd78bd3e3bd1e9155dab4/src/generated/next_generated.js#L10-L15.

Maybe after Soroban on pubnet!

@wildework
Copy link
Author

It's cool that set is gone and no longer creating issues. Thanks for the update!

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

No branches or pull requests

4 participants