-
Notifications
You must be signed in to change notification settings - Fork 160
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
MA Policies Whitelist #1610
MA Policies Whitelist #1610
Conversation
e7c17e7
to
33f2272
Compare
66a50cf
to
f5c2028
Compare
85527d3
to
44b6d0b
Compare
21d0f5c
to
af8c468
Compare
plutusMultiAssetWhitelistCheck :: SyncEnv -> [Generic.TxOut] -> Bool | ||
plutusMultiAssetWhitelistCheck syncEnv txOuts = | ||
plutusMaybeCheck txOuts && (plutusWhitelistCheck syncEnv txOuts || multiAssetWhitelistCheck syncEnv txOuts) | ||
plutusMaybeCheck txOuts || (plutusWhitelistCheck syncEnv txOuts || multiAssetWhitelistCheck syncEnv txOuts) |
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.
plutusWhitelistCheck
is called in plutusMaybeCheck
, so why is it also called 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 can't see plutusWhitelistCheck
inside plutusMaybeCheck
, am I understanding you right?
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.
Oh sorry I got confused by indentation.
Actually I don't see what's the purpose of plutusMaybeCheck
. It may return False
even with PlutusEnable
, which should never happen. plutusWhitelistCheck
already does the necessary work.
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.
The was my original comment
-- TODO: cmdv: unsure if this is correct because if plutusMaybeCheck fails then no multiasset whitelist is not checked
I was trying to make what we spoke about and push the checks as far up the chain and I may have miss understood what you meant and that's why I left this comment there about the plutusMaybeCheck
. It's there as a check so you don't do any whitelist checks if there isn't any txOutScript
or txOutAddress
and that's what it was an &&
not an ||
so this is:
if plutusMaybeCheck txOuts
then (plutusWhitelistCheck syncEnv txOuts || multiAssetWhitelistCheck syncEnv txOuts)
else False
which is essentially bellow as it will return false straight away if plutusMaybeCheck txOuts
if false:
plutusMaybeCheck txOuts && (plutusWhitelistCheck syncEnv txOuts || multiAssetWhitelistCheck syncEnv txOuts)
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 still can't fully understand what does plutusMaybeCheck
check, since all necessary checks are done in plutusWhitelistCheck
. Using or
instead of and
here will simply give True
in more cases than we want to.
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 can just remove plutusMaybeCheck
, unless I'm missing something
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 was just to save doing big elem
checks if they're not needed. But it's all good we can just do it inside the whitelist checks.
af8c468
to
6a1a323
Compare
6dc9215
to
65c2fe2
Compare
65c2fe2
to
41a1fce
Compare
closing this PR as it's been superseded by #1644 |
Description
This is the first part of #1600 adding a whitelist for MA Policies and renaming:
KeepMetadataNames
->WhitelistMetadataNames
Checklist
fourmolu
on version 0.10.1.0 (which can be run withscripts/fourmolize.sh
)Migrations
If there is a breaking change, especially a big one, please add a justification here. Please elaborate
more what the migration achieves, what it cannot achieve or why a migration is not possible.