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

Use reference input #4114

Merged
merged 19 commits into from
Sep 18, 2023
Merged

Conversation

paweljakubas
Copy link
Contributor

@paweljakubas paweljakubas commented Aug 29, 2023

  • update ApiMinBurnFromInput to accommodate policy id
  • update swagger and spec
  • regenerate golden and make sure unit tests pass
  • incorporate new ApiMinBurnData inside constructTransaction
  • introduce ScriptSource
  • handle from input case to mkUnsignedTx
  • adjust mkUnsignedTx
  • show the case in integration testing`

Comments

builds on top of #4086

Issue Number

adp-3090

@paweljakubas paweljakubas self-assigned this Aug 29, 2023
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-3090/use-reference-input branch 3 times, most recently from 8298420 to e299647 Compare September 1, 2023 13:44
@paweljakubas paweljakubas marked this pull request as ready for review September 5, 2023 13:59
Comment on lines 191 to 200
-- when wallets uses reference input it means script containing
-- its policy key was already published in previous tx
-- if so we need to add one witness that will stem from policy signing
-- key. As it is not allowed to publish and consume in the same transaction
-- we are not going to double count.
txRefInpsWit =
case Cardano.txInsReference txbodycontent of
Cardano.TxInsReferenceNone -> 0
Cardano.TxInsReference _ _ -> 1
Copy link
Member

@Anviking Anviking Sep 5, 2023

Choose a reason for hiding this comment

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

This seems like it's assuming each reference input points to an inline script which requires 1 key witness? Otherwise I don't understand it.

What if the script requires two key witnesses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if there is reference input(s) then each reference input refers to tx where script was published. Each script has structure : [cosigner#0, timelocks] and it means it contains policy key. We use ONE policy key per wallet, which means there is going to be ONE witness from this or ZERO. So Whatever number of reference inputs,one witness with wallet's policy signing key is going to be used.

Remember that we are here in SHELLEY and script regulates minting/burning. So yes, we will only have ONE cosigner. In shared style it would be different but atm we are not supporting minting/burning for shared wallets.

Copy link
Contributor

@HeinrichApfelmus HeinrichApfelmus Sep 15, 2023

Choose a reason for hiding this comment

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

In the presence of Plutus validator scripts, the number of extra witnesses can arbitrarily depend on the data in the reference inputs, so the estimate can always fail terribly in principle.

But even in the absence of Plutus validator scripts, I think that the presence of reference inputs alone is not a good basis for estimation. 🤔 What about combining this with Cardano.txMintValue? The logic being that if the transaction mints something, and there is a reference input, then the most likely of that input is to contain a script which requires one additional witness.

      case Cardano.txInsReference txbodycontent of
          Cardano.TxInsReferenceNone -> 0
          Cardano.TxInsReference{} ->
              case CardanotxMintValue txbodycontent of
                  Cardano.TxMintValueNone -> 0
                  Cardano.TxMintValue{} -> 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! Very good suggestion! Adopted!

@paweljakubas paweljakubas force-pushed the paweljakubas/adp-3090/use-reference-input branch 3 times, most recently from 885b037 to 4b83dca Compare September 7, 2023 09:57
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-3090/use-reference-input branch 4 times, most recently from 3a96eab to f9d6853 Compare September 14, 2023 10:52
Copy link
Contributor

@HeinrichApfelmus HeinrichApfelmus 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 your work! 😊 I have two requests that I would like to see addressed, but good to go afterwards.

let (ApiPolicyId (ApiT policyId')) = getFromResponse Prelude.id rGet

eventually "wb wallet has received funds" $ do
request @ApiWallet ctx (Link.getWallet @'Shelley wb) Default Empty
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change this to check for the status of the transaction instead, like we discussed some time ago for another integration test? (I forgot which one)

I.e. instead of checking whether a balance has increased, we check whether this particular transactions has been accepted into the ledger by querying the getTransaction endpoint with the transaction id and waiting until the status is in_ledger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, yes. done

@paweljakubas paweljakubas force-pushed the paweljakubas/adp-3090/use-reference-input branch from f9d6853 to 9498a85 Compare September 15, 2023 15:25
Copy link
Contributor

@HeinrichApfelmus HeinrichApfelmus left a comment

Choose a reason for hiding this comment

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

Thanks! 😊

@paweljakubas paweljakubas added this pull request to the merge queue Sep 18, 2023
Merged via the queue into master with commit 6157932 Sep 18, 2023
2 checks passed
@paweljakubas paweljakubas deleted the paweljakubas/adp-3090/use-reference-input branch September 18, 2023 10:22
WilliamKingNoel-Bot pushed a commit that referenced this pull request Sep 18, 2023
…the work accomplished in this PR. Before you submit, don't forget to: CODE-OF-CONDUCT.md CONTRIBUTING.md LICENSE MAINTAINERS.md README.md cabal.project default.nix docker-compose.yml docs flake.lock flake.nix floskell.json fourmolu.yaml hie-direnv.yaml justfile lib nix prototypes reports scripts shell.nix specifications test touch.me.CI weeder.dhall Make sure the GitHub PR fields are correct: ✓ Set a good Title for your PR. ✓ Assign yourself to the PR. ✓ Assign one or more reviewer(s). ✓ Link to a Jira issue, and/or other GitHub issues or PRs. ✓ In the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. CODE-OF-CONDUCT.md CONTRIBUTING.md LICENSE MAINTAINERS.md README.md cabal.project default.nix docker-compose.yml docs flake.lock flake.nix floskell.json fourmolu.yaml hie-direnv.yaml justfile lib nix prototypes reports scripts shell.nix specifications test touch.me.CI weeder.dhall Don't waste reviewers' time: ✓ If it's a draft, select the Create Draft PR option. ✓ Self-review your changes to make sure nothing unexpected slipped through. CODE-OF-CONDUCT.md CONTRIBUTING.md LICENSE MAINTAINERS.md README.md cabal.project default.nix docker-compose.yml docs flake.lock flake.nix floskell.json fourmolu.yaml hie-direnv.yaml justfile lib nix prototypes reports scripts shell.nix specifications test touch.me.CI weeder.dhall Try to make your intent clear: ✓ Write a good Description that explains what this PR is meant to do. ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding Jira ticket. ✓ Highlight what Testing you have done. ✓ Acknowledge any changes required to the Documentation. --> - [x] update `ApiMinBurnFromInput` to accommodate policy id - [x] update swagger and spec - [x] regenerate golden and make sure unit tests pass - [x] incorporate new `ApiMinBurnData` inside `constructTransaction` - [x] introduce ScriptSource - [x] handle from input case to `mkUnsignedTx` - [x] adjust `mkUnsignedTx` - [x] show the case in integration testing` ### Comments builds on top of #4086 <!-- Additional comments, links, or screenshots to attach, if any. --> ### Issue Number adp-3090 <!-- Reference the Jira/GitHub issue that this PR relates to, and which requirements it tackles. Note: Jira issues of the form ADP- will be auto-linked. --> Source commit: 6157932
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.

3 participants