-
Notifications
You must be signed in to change notification settings - Fork 84
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
Depend on plutus-ledger and plutus-script-utils #1656
base: master
Are you sure you want to change the base?
Conversation
locallycompact
commented
Sep 26, 2024
- CHANGELOG updated or not needed
- Documentation updated or not needed
- Haddocks updated or not needed
- No new TODOs introduced or explained herafter
@@ -107,20 +116,21 @@ validator (_party, _commit, headId) r ctx = | |||
headOutputValue = | |||
txOutValue . head $ txInfoOutputs (scriptContextTxInfo ctx) | |||
|
|||
compiledValidator :: CompiledCode ValidatorType | |||
compiledValidator :: TypedValidator CommitValidator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC this was adding some overhead to the plutus scripts and that's why we ended up with "plain type aliases" on DatumType
and RedeemerType
.
Might not be the case anymore, but we should be cautious.
validatorHash :: ScriptHash | ||
validatorHash = scriptValidatorHash PlutusScriptV2 validatorScript | ||
validatorHashAddress :: Address | ||
validatorHashAddress = Ledger.scriptValidatorHashAddress validatorHash' Nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do not need Address
values for our script as we only would need to depend on the script hashes (ValidatorHash
now?)
-- NOTE: Ada in initialValue is usually lower than in the locked ADA due | ||
-- NOTE: Ada in initialValue is usually lower than in the locked ADA due | ||
-- to higher deposit needed for commit output than for initial output | ||
-- to higher deposit needed for commit output than for initial output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something's wrong with your editor or tools here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest not doing this at the same time. Most likely, not using this custom script context will blow up the execution unit cost and makes us unable to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ScriptContext is everywhere in plutus-script-utils, so we would not be able to drop our functions. Maybe making sure we have the same module interface as plutus-script-utils is a good idea so we can swap one for the other ongoing.
--sha256: sha256-bhaJsWqjhojpbwxLX1+vVPSPz86tbhDcBUHk6M1HPzU= | ||
subdir: | ||
plutus-script-utils | ||
plutus-ledger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use the packages from CHaP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't merged yet because it's a monorepo that would require fixes to cardano-node-enulator as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have raised IntersectMBO/cardano-node-emulator#38