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

Automatically reduce 0 and empty multiassets #372

Merged
merged 15 commits into from
Sep 7, 2024
Merged

Conversation

nielstron
Copy link
Contributor

I noticed that the new ledger rules now disallow empty dicts and 0 values in multiassets. It would be good to anyways prevent this by design - this also always makes transactions cheaper. Here is an implementation proposal. I also tried to verify the non-emptiness strictness with an integration test but it seems to fail. We may want to remove it.

@nielstron nielstron requested a review from cffls September 2, 2024 20:28
Copy link

codecov bot commented Sep 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.33%. Comparing base (7243cd9) to head (b6e6dea).
Report is 1 commits behind head on chang.

Additional details and impacted files
@@            Coverage Diff             @@
##            chang     #372      +/-   ##
==========================================
+ Coverage   85.23%   85.33%   +0.09%     
==========================================
  Files          32       32              
  Lines        4153     4194      +41     
  Branches     1040     1055      +15     
==========================================
+ Hits         3540     3579      +39     
- Misses        428      429       +1     
- Partials      185      186       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nielstron nielstron mentioned this pull request Sep 2, 2024
5 tasks
@nielstron
Copy link
Contributor Author

This PR does also not yet cover the possibility of creating a 0 value by hand (i.e. accidentally by adding stuff together)

@nielstron
Copy link
Contributor Author

@cffls Can you maybe help with that last bit?
I think the fee computation may be thrown off somewhere - are we computing the wrong number of tokens?

FAILED test/test_zero_empty_asset.py::TestZeroEmptyAsset::test_submit_zero_and_empty - ogmios.errors.ResponseError: Ogmios responded with error: {'jsonrpc': '2.0', 'method': 'submitTransaction', 'error': {'code': 3122, 'message': "Insufficient fee! The transaction doesn't not contain enough fee to cover the minimum required by the protocol. Note that fee depends on (a) a flat cost fixed by the protocol, (b) the size of the serialized transaction, (c) the budget allocated for Plutus script execution. The field 'data.minimumRequiredFee' indicates the minimum required fee whereas 'data.providedFee' refers to the fee currently supplied with the transaction.", 'data': {'minimumRequiredFee': {'ada': {'lovelace': 189261}}, 'providedFee': {'ada': {'lovelace': 184861}}}}, 'id': None}

User can potentially provide signing keys that are not required by the finalized transaction.
For example, if a transaction include a minting script and signing key for the script, but the minted value is 0, the signature of the minting script isn't required.
This commit will auto detect such cases and skip the signing of this key.
@nielstron
Copy link
Contributor Author

Great, thank you @cffls
Will merge this into chang then unless you have further comments

@cffls
Copy link
Collaborator

cffls commented Sep 7, 2024

Looks good to me! Please feel free to merge.

@nielstron nielstron merged commit fb57ed4 into chang Sep 7, 2024
11 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.

2 participants