Emurgo / cardano-serialization-lib

This is a library, written in Rust, for serialization & deserialization of data structures used in Cardano's Haskell implementation of Alonzo along with useful utility functions.
Other
231 stars 125 forks source link

TransactionBuilder.set_certs() throws RuntimeError: unreachable #623

Closed zxpectre closed 1 year ago

zxpectre commented 1 year ago

Hi! I am adding multisig support on all transaction features. On TransactionBuilder.set_certs() (2 certs with the same native script stake credential), when trying to combine Registration and Delegation on a single transaction it fails with RuntimeError: unreachable. The issue appears on the Delegation certificate, the second on the provided list.

When I only provide the Registration certificate (first one), transaction succeeds, so I guess this certificates are properly constructed. I have succeded before combining Registration and Delegation certificates atomically on a single transaction using KeyHash credentials

Certificates instance in hexadecimal:

8282008201581c4658726c25d0b50454c143647a2d138a1f1727fa5ee8c3a6c75f609883028201581c4658726c25d0b50454c143647a2d138a1f1727fa5ee8c3a6c75f6098581cdb1018753659a073c43ad75c1f243f97a3896a45b81a61300ea96a9f

Certificates instance in json schema 1:

[
  {
    "StakeRegistration": {
      "stake_credential": {
        "Script": "4658726c25d0b50454c143647a2d138a1f1727fa5ee8c3a6c75f6098"
      }
    }
  },
  {
    "StakeDelegation": {
      "stake_credential": {
        "Script": "4658726c25d0b50454c143647a2d138a1f1727fa5ee8c3a6c75f6098"
      },
      "pool_keyhash": "db1018753659a073c43ad75c1f243f97a3896a45b81a61300ea96a9f"
    }
  }
]

Using "@emurgo/cardano-serialization-lib-browser": "^11.3.1", Testing in Chrome and Pre-Prod Testnet

lisicky commented 1 year ago

@zxpectre Does it fail when you interact with CSL or when you try to submit the tx ?

zxpectre commented 1 year ago

With CSL, more specifically set_certs() is throwing that error. Searching on google I found that it can be related to Rust/Wasm:

ex 1 ex 2

lisicky commented 1 year ago

@zxpectre could you share a code example to reproduce it ?

zxpectre commented 1 year ago

@zxpectre could you share a code example to reproduce it ?

Sorry @lisicky , I don't have much time now to make a full working code, this minimum lines reproduce it.

    let certificatesObj=CardanoWasm().Certificates.from_hex("8282008201581c4658726c25d0b50454c143647a2d138a1f1727fa5ee8c3a6c75f609883028201581c4658726c25d0b50454c143647a2d138a1f1727fa5ee8c3a6c75f6098581cdb1018753659a073c43ad75c1f243f97a3896a45b81a61300ea96a9f");
    txBuilder.set_certs(certificatesObj);

I've cleaned/reinstall node_modules, tried with only the Delegation cert on the list, causes the same error, (Only scripthash delegation cert seems to fail, same poolhash works with any other keyhash based stake Delegation cert) tryied several other ways and no luck, same error. No other parts of the code had presented weird errors like this one so far, on a working product.

Here you have a successfull Stake Registration transaction CBOR with that script credential, proving at least Registration cert is working.

CBOR

Block Explorer

Does anyone have a codepen/jsBin ready template for CSL around?

zxpectre commented 1 year ago

@lisicky I've made this repo to replicate the error under similar runtime conditions https://github.com/zxpectre/react-csl/blob/runtimeErrorUnreachable/src/app/main.js you can npm run start on your own.

Maybe I'm missing something? image

lisicky commented 1 year ago

@zxpectre thanks for your code! CSL doesn't support certificates with a script stake credentials. I'm going to discuss with team about it. And I'll come back in two days with answer when we will fix it

zxpectre commented 1 year ago

ok @lisicky, that would be very appreciated!

Maybe you don't know it yet but CSL powers GameChanger Wallet internal tx builder since 2021 Q2, with the v2 beta we are almost exporting your entire tx builder API to became accesible to users using low code JSON based scripting and URL links. I am so close to finally offer native full multisig support but I totally depend on you guys here.

So far script driven (multisig?) minting, spending works natively on GC UI and API, probably reward withdrawal of script reward addresses work as well (about to test it once delegation can be accomplished).

may-evidence-gcfs-nft-creator-smart-send-for-daos

lisicky commented 1 year ago

Hi @zxpectre ! I'll make a new CertificatesBuilder with script script stake credentials support. It will take around 1 week, but I can't predict a new CSL version release date now. But anyway for test you will able to build lib from the source code.

zxpectre commented 1 year ago

Thanks @lisicky !! much appreciated! Will show you the resulting implementation as soon as I can test this out if you are interested.

You expect to introduce any breaking changes on class and method definitions here?

lisicky commented 1 year ago

@zxpectre I have created PR. You can try it on your side https://github.com/Emurgo/cardano-serialization-lib/pull/628 . The PR doesn't have any breaking changes, there are new classes CertificatesBulilder and WithdrawalsBuilder

zxpectre commented 1 year ago

Thanks @lisicky !!! I've been busy, will be trying it out this week and get back with feedback.

lisicky commented 1 year ago

Hi @zxpectre ! Did you have a chance to check it ?

zxpectre commented 1 year ago

Hi @lisicky ! I have been very busy, but now it's next on my TODO list. Will come back to you for sure.

zxpectre commented 1 year ago

@lisicky I've build your branch for browser and test it against this code on React-CSL with the same certificate, and seems to work ok.

    const scriptHex     ="830302868200581c3ecb73dee12d39d8787c9b87e71e857c6dd487d4fe7c5173e0bb8c748200581c1d46f358ba291245db79a6b953703a131e688ab2054e6a85b742d6218200581cad0a0ecf8ae61aba9b75d337b7348a4ad5d1a92aad9d8aeaf04b28d28200581c1a4c58b1374494657edefc5b8931c3a2299935d67f44d4db87ae902d8200581c898c5d9392158c9cf5dfb6576a083db1ce7b08f785c8976e50e09a128200581cb0dabc722178ff3494e21c376b080294af8f565e66fa283e14424fb5"
    const scriptHash    ="4658726c25d0b50454c143647a2d138a1f1727fa5ee8c3a6c75f6098";
    const poolKeyHash   ="7facad662e180ce45e5c504957cd1341940c72a708728f7ecfc6e349";
    const scriptObj     =CardanoWasm.NativeScript.from_hex(scriptHex);
    let certificateObj  = CardanoWasm.Certificate.new_stake_delegation(
        CardanoWasm.StakeDelegation.new(
            CardanoWasm.StakeCredential.from_scripthash (CardanoWasm.ScriptHash     .from_hex(scriptHash) ),
            CardanoWasm.Ed25519KeyHash.from_hex(poolKeyHash)
        ),
    );
    logger("log"    ,"certificateObj ok");
    const certBuilderObj=CardanoWasm.CertificatesBuilder.new();
    certBuilderObj.add_with_native_script(certificateObj,CardanoWasm.NativeScriptSource.new(scriptObj))
    logger("log"    ,"certBuilderObj ok");
    certBuilderObj.build();
    logger("log",`certBuilderObj.build() ok`);
    txBuilder.set_certs_builder(certBuilderObj);
    logger("log"    ,"set_certs_builder ok");

image

Also checked out the proper error message regarding the previous deprecated method.

For actually integrating this on the wallet, it will take a while for me to focus and update properly my tx builder code to use your new CertificatesBuilder with native and plutus scripts.

Actually I hope to even use those helper methods that allows one to calculate deposits and refunds, thanks for doing this! Is very handy, and much needed! Was doing everything manually before so now I need to refactor more.

Any idea when this will be included on next release?

Big thanks from our team, will keep working on it and probably return to you with more feedback

zxpectre commented 1 year ago

Hi @lisicky! I've been integrating all this classes and methods into the wallet tx builder.

I'm gathering some feedback and questions, but now the most urgent issue I spotted is that

certificatesBuilder.add_with_native_script (certObj,nativeScriptSourceObj); throws "Your certificate does not have a required script witness.Please use .add instead." on this certificate:

      "registrationCert": {
        "kind": "StakeRegistration",
        "scriptHashHex": "1fc0f80586a89f78bf9779c35af0c2f2e0e80216f77c31ca77a1a403",
        "certHex": "82008201581c1fc0f80586a89f78bf9779c35af0c2f2e0e80216f77c31ca77a1a403",
        "scriptHex": "830302868200581c3ecb73dee12d39d8787c9b87e71e857c6dd487d4fe7c5173e0bb8c748200581c1d46f358ba291245db79a6b953703a131e688ab2054e6a85b742d6218200581cad0a0ecf8ae61aba9b75d337b7348a4ad5d1a92aad9d8aeaf04b28d28200581cae0a3f747a760105b79dde3b46d37b81568568a86c7f4822e29504cb8200581c385c81a738af1779d645ba36c6108053c78516308af5af71131c71f38200581c9c9f4c9f088af2503515ca446e213573770f7d152b92491b3fb4da8c"
      }
lisicky commented 1 year ago

Hi @zxpectre ! Huge thanks for checking PR! You need to provide a script witness only for StakeDeregistration or StakeDelegation certificate

zxpectre commented 1 year ago

@lisicky should txBuilder.set_certs_builder(certificatesBuilderObj) call internally not only certificatesBuilderObj.build() for getting cert list but also certificatesBuilderObj.get_certificates_deposit(..) and certificatesBuilderObj.get_certificates_refund(..) to use their results later when calling txBuilder.add_change_if_needed(changeAddressObj)?

Im asking because .add_change_if_needed() is not taking into account deposits (and maybe nor refunds) and I think one of the reasons for doing the builders may be also to assist internally in fee and change calcs right?

What's strange is that txBuilder.get_deposit() is returning the right value, only .add_change_if_needed() is not using it.

Also, less importantly, why .get_certificates_refund() returns a Value and .get_certificates_deposit() a BigNum and not the same type both?

zxpectre commented 1 year ago

Update: I've enabled our tx fee patcher to help with this issue and here are the first results.

Script certificates support on our dapp connector: https://youtu.be/cJZlU_TmZWU

Congratulations @lisicky ! Much more is coming as this triggers lot more of work on our side

lisicky commented 1 year ago

@zxpectre how did you check that .add_change_if_needed() is not taking into account deposits ? Your app connector looks cool : )

zxpectre commented 1 year ago

@zxpectre how did you check that .add_change_if_needed() is not taking into account deposits ? Your app connector looks cool : )

Thanks @lisicky ! You could be using it for building and testing ;)

I'm super delayed to make you some examples, (only active dev on team since last year) but I had to use a fee patcher to make them work, and trust me, they work with the corrected fee.

Now I'm facing this using this branch

lisicky commented 1 year ago

@zxpectre I have checked on my side and everything seems correct that's why I asked about how you did check it .