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

Chang support #371

Merged
merged 48 commits into from
Sep 24, 2024
Merged

Chang support #371

merged 48 commits into from
Sep 24, 2024

Conversation

nielstron
Copy link
Contributor

@nielstron nielstron commented Sep 2, 2024

Creating this thread to collect discussion about Chang support. Superseded #368

TODO

  • add tests for ogmiosv6backend tx fetching
  •  add ogmiosv6backend support for plutusv3
  • add tests for kupo backend tx fetching
  •  disallow 0 and non-empty assets / values
  •  kupo plutus v3 support

Copy link
Collaborator

@cffls cffls left a comment

Choose a reason for hiding this comment

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

I think this is in a good state to merge. @nielstron do you have anything to add?

@nielstron
Copy link
Contributor Author

LGTM

@theeldermillenial
Copy link
Contributor

As a heads up, I am testing this out tonight. It does not appear as though some wallets (i.e. Eternl) supports redeemers as maps yet. I am getting errors when generating cbor with pycardano and then sending to Eternl for signing.

Yes, I think this is an Eternl problem, but I did want to put this on the map for anyone else potentially running into this.

@nielstron
Copy link
Contributor Author

I experienced something similar with Mesh. But Eternal works with platforms like MinSwap, so they do support some form of SC txs that is accepted on Chang? Are we missing something?

@theeldermillenial
Copy link
Contributor

Can confirm that this is the problem.

Emurgo/cardano-serialization-lib#693

Right now it seems CSL only uses array style redeemers. So this could likely affect multiple wallets.

@nielstron
Copy link
Contributor Author

nielstron commented Sep 21, 2024

According to this comment we should still allow Redeemers as array: Emurgo/cardano-serialization-lib#693 (comment)

Maybe add a switch to the txbuilder for this? @cffls can you look into this since you also added the map support in the first place?

@theeldermillenial
Copy link
Contributor

theeldermillenial commented Sep 21, 2024

I did some looking into it but haven't had a chance to try it out.

I think there's a one line work around. The Redeemer type is fortunately defined as a list or map. The original redeemer type is still defined, the map was added to it.

I think the workaround looks like this:

  1. Build tx with txbuilder
  2. Use a list comprehension to convert redeemer map to list

Obviously if you're building the tx manually you can just set the redeemers as an array.

An alternative solution would be to add an option to the txbuilder that specifies redeemer format. By default, it can be set to map, but can be changed to list. Should also be an easy thing to implement.

I'll be able to test out in the next few hours. If you have a comment on this approach, please let me know.

@nielstron
Copy link
Contributor Author

This sounds great @theeldermillenial ! Either approach is fine, maybe just add a function "redeemer_map_to_list" and use that when a flag is set in .build? So both manual builders and the automated builders can benefit from this.

@theeldermillenial
Copy link
Contributor

theeldermillenial commented Sep 21, 2024

That's a good idea. I'll try it out.

@theeldermillenial
Copy link
Contributor

Alright, heres my code to swap out the redeemer maps to lists:

    if (
        tx.transaction_witness_set.redeemer is not None
        and len(tx.transaction_witness_set.redeemer) > 0
    ):
        tx.transaction_witness_set.redeemer = [
            pycardano.Redeemer.from_primitive(k.to_primitive() + v.to_primitive())
            for k, v in tx.transaction_witness_set.redeemer.items()
        ]

The problem I'm having is that Eternl can't seem to read the cbor after this. I tested building the cbor both with maps and lists, dumping to cbor, and importing into Eternl. In both cases, it will pull up the tx, but if you look at the witness set, it's basically full of null values.

I'll keep hacking away at it, but it looks like this might be more complex than I initially expected.

@theeldermillenial
Copy link
Contributor

Nix that, this seems to work. I'll submit a PR today. I had something that was out of alignment with the change branch.

@cffls
Copy link
Collaborator

cffls commented Sep 21, 2024

Txbuilder always keeps a list of redeemers internally, so the conversion might not be needed. I will try and see if there is a simpler solution to this.

@cffls
Copy link
Collaborator

cffls commented Sep 21, 2024

Pushed a commit for the fix.
Now the serialization of redeemers could be configured by setting field use_redeemer_map in txbuilders.

@theeldermillenial
Copy link
Contributor

theeldermillenial commented Sep 22, 2024

@cffls @nielstron

I actually found an additional error that was causing a different issue in Eternl.

When the vkey_witnesses is empty, it encodes an empty value in the map. This causes a problem in Eternl (and I assume this might be a CSL issue) where the entire witness set is read incorrectly. Instead, what is needed is a catch to set vkey_witnesses=None when len(vkey_witnesses)==0.

I use empty signing keys when building the transaction with txbuilder.

In Eternl, when interacting with the extension through our frontend, we still get the error about redeemers using a map instead of an array, but now I can successfully import the same transaction into Eternl and submit it.

I'll pull in the latest changes and test that along with my fix.

It'll be tomorrow though because I've been at this and a few other days and the brain is fried.

@theeldermillenial
Copy link
Contributor

Alright, between the fix from Jerry and this, everything seems to be running smooth.

…376)

* Fixed tx imbalance when burning multiple tokens

* Added cases and format

* Added unit test

* Correct unit test

* Removed redundant filtering zero-quantity asset

* Improved name function (unit test)

* Improved unit test

* Fix __iadd__ in assets

---------

Co-authored-by: Niels <[email protected]>
Co-authored-by: Jerry <[email protected]>
@cffls cffls merged commit 7f22ba7 into main Sep 24, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants