-
Notifications
You must be signed in to change notification settings - Fork 213
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
integration tests: Mint assets in the cluster faucet, and add basic MA scenarios #2465
Conversation
da02cde
to
b7bf6eb
Compare
b7bf6eb
to
a060bd3
Compare
let TokenBundle.AssetId pid an = Set.elemAt 0 (TokenMap.getAssets assetsSrc) | ||
let val = [(toText pid, toText an, minUTxOValue)] | ||
|
||
payload <- liftIO $ mkTxPayloadMA ctx wDest minUTxOValue val fixturePassphrase |
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.
Seeing some strange things removing the pendingWith
after rebasing (not sure it is the same as what you, @rvl saw):
As is here, specifying minUTxOValue
ada
Wallet error: InsufficientMinCoinValueError: 1 ada, expectedMinCoinValue=1.4 ada
"{\\\"code\\\":\\\"utxo_too_small\\\",\\\"message\\\":\\\"Some outputs have ada values that are too small. There's a minimum ada value specified by the protocol that each output must satisfy. I'll handle that minimum value myself when you do not explicitly specify an ada value for an output. Otherwise, you must specify enough ada. Here are the problematic outputs: InsufficientMinCoinValueError {outputWithInsufficientAda = TxOut {address = Address {unAddress = \\\\\\\"\\\\\\\\SOH\\\\\\\\238\\\\\\\\229Q\\\\\\\\178\\\\\\\\149\\\\\\\\244\\\\\\\\165`\\\\\\\\169Z\\\\\\\\250\\\\\\\\208AC@\\\\\\\\vx\\\\\\\\165\\\\\\\\242\\\\\\\\f\\\\\\\\254\\\\\\\\196\\\\\\\\232\\\\\\\\160[\\\\\\\\225kQ\\\\\\\\197\\\\\\\\196\\\\\\\\ETX\\\\\\\\148\\\\\\\\185/\\\\\\\\186\\\\\\\\&7S\\\\\\\\179\\\\\\\\153Ao\\\\\\\\EM\\\\\\\\221\\\\\\\\SO\\\\\\\\191\\\\\\\\239\\\\\\\\251\\\\\\\\173\\\\\\\\131\\\\\\\\176\\\\\\\\236\\\\\\\\DLE\\\\\\\\234+q\\\\\\\\a\\\\\\\"}, tokens = TokenBundle {coin = Coin 1000000, tokens = TokenMap (fromList [(UnsafeTokenPolicyId (Hash \\\\\\\"K\\\\\\\\254z\\\\\\\\202\\\\\\\\225\\\\\\\\189%\\\\\\\\153d\\\\\\\\153b\\\\\\\\177F\\\\\\\\161\\\\\\\\228}.\\\\\\\\DC4\\\\\\\\147\\\\\\\\&8\\\\\\\\t\\\\\\\\179g\\\\\\\\232\\\\\\\\EOT\\\\\\\\198\\\\\\\\US\\\\\\\\134\\\\\\\"),NonEmptyMap {least = (UnsafeTokenName \\\\\\\"\\\\\\\",TokenQuantity 1000000), rest = fromList []})])}}, expectedMinCoinValue = Coin 1407406} :| []\\\"}\", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}")
When Specifying 0
ada
Ledger error: OutputTooSmallUTxO: 1 ada
from the following response: Left (DecodeFailure "{\"code\":\"created_invalid_transaction\",\"message\":\"That's embarrassing. It looks like I've created an invalid transaction that could not be parsed by the node. Here's an error message that may help with debugging: HardForkApplyTxErrFromEra S (S (S (Z (WrapApplyTxErr {unwrapApplyTxErr = ApplyTxError [LedgerFailure (UtxowFailure (UtxoFailure (OutputTooSmallUTxO [(Addr Mainnet (KeyHashObj (KeyHash \\\"e4674505240c98422ad66a98ecdaca42770fa7c66a2bf1f81048c5ad\\\")) (StakeRefBase (KeyHashObj (KeyHash \\\"211fa28b02df352f6a5e93f9c920b967e5359180cb36b499093647f3\\\"))),Value 1407406 (fromList [(PolicyID {policyID = ScriptHash \\\"4bfe7acae1bd2599649962b146a1e47d2e14933809b367e804c61f86\\\"},fromList [(\\\"\\\",1000000)])]))])))]}))))\"}")
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.
That second error is quite bad. Sounds like something isn't working properly. When specyfing 0 Ada, the server should assign the minimum ada value needed for that UtxO. And, the first error suggests that the server is perfectly able to compute that minimum value because it suggests it in the error message. So, something is off..
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.
As is here, specifying minUTxOValue ada
As far as I understand it is expected, since minUTxOValue
may be different for MA. (Correct me if I'm wrong)
When Specifying 0 ada
Interesting, because I don't see it on mary_qa
. When specifying 0 ADA on tx with MA the transaction is given minUTxOValue
correctly and tx is succesful.
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 first case is indeed expected, and the error returned by the server looks legit (1.4 Ada of expected min coin value). The second error is puzzling. From the message however, we can't really see if the error is on the change output or on the requested 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.
The "expected" min ada behaviour is a little odd, but I have amended the test cases accordingly. Perhaps the HTTP status code should be 400 Bad Request instead of 403 Forbidden.
I can't reproduce the second error in the integration tests.
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 "expected" min ada behaviour is a little odd
What do you mean? Do you find the server behavior odd, or the ledger requirement odd?
Perhaps the HTTP status code should be 400 Bad Request instead of 403 Forbidden.
That's a semantic debate, yet I do think of 400 Bad Request
as requests which are malformed (improper JSON, invalid types, missing required keys etc..). Though, if the request is syntactically valid, but only present an invalid combination of inputs, then I rather go for 403.
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 reproduce the second error in the integration tests.
You mean that, passing a value of 0 Ada has the intended behavior (that is, min ada value is automatically assigned, and the transaction go through) ?
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.
Oops, I think the UtxoFailure (OutputTooSmallUTxO
I saw was since I was still on 1.24.2 locally.
Now, with 1.25.1 I only get
src/Test/Integration/Scenario/API/Shelley/Transactions.hs:227:13:
1) API Specifications, SHELLEY_TRANSACTIONS, Regression ADP-626 - Filtering transactions between eras
expected: 1
but got: 2
To rerun use: --match "/API Specifications/SHELLEY_TRANSACTIONS/Regression ADP-626 - Filtering transactions between eras/"
src/Test/Integration/Scenario/API/Shelley/Transactions.hs:701:22:
2) API Specifications, SHELLEY_TRANSACTIONS, TRANS_CREATE_10 - Multi-asset transaction with Ada
While verifying (Status {statusCode = 200, statusMessage = "OK"},Right (ApiWallet {id = ApiT {getApiT = WalletId {getWalletId = 56c7a5f8489970d793363da6b6cad018ce8e8af4}}, addressPoolGap = ApiT {getApiT = AddressPoolGap {getAddressPoolGap = 20}}, balance = ApiWalletBalance {available = Quantity {getQuantity = 2000000}, total = Quantity {getQuantity = 2000000}, reward = Quantity {getQuantity = 0}}, assets = ApiWalletAssetsBalance {available = ApiT {getApiT = TokenMap (fromList [(UnsafeTokenPolicyId (Hash "K\254z\202\225\189%\153d\153b\177F\161\228}.\DC4\147\&8\t\179g\232\EOT\198\US\134"),NonEmptyMap {least = (UnsafeTokenName "",TokenQuantity 1000000), rest = fromList []})])}, total = ApiT {getApiT = TokenMap (fromList [(UnsafeTokenPolicyId (Hash "K\254z\202\225\189%\153d\153b\177F\161\228}.\DC4\147\&8\t\179g\232\EOT\198\US\134"),NonEmptyMap {least = (UnsafeTokenName "",TokenQuantity 1000000), rest = fromList []})])}}, delegation = ApiWalletDelegation {active = ApiWalletDelegationNext {status = NotDelegating, target = Nothing, changesAt = Nothing}, next = []}, name = ApiT {getApiT = WalletName {getWalletName = "Empty Wallet"}}, passphrase = Just (ApiWalletPassphraseInfo {lastUpdatedAt = 2021-01-29 09:26:00.183826 UTC}), state = ApiT {getApiT = Ready}, tip = ApiBlockReference {absoluteSlotNumber = ApiT {getApiT = SlotNo 551}, slotId = ApiSlotId {epochNumber = ApiT {getApiT = EpochNo {unEpochNo = 11}}, slotNumber = ApiT {getApiT = SlotInEpoch {unSlotInEpoch = 1}}}, time = 2021-01-29 09:27:29.2 UTC, block = ApiBlockInfo {height = Quantity {getQuantity = 234}}}}))
Waited longer than 90s to resolve action: "Payee wallet balance is as expected".
expected: 1000000
but got: 2000000
To rerun use: --match "/API Specifications/SHELLEY_TRANSACTIONS/TRANS_CREATE_10 - Multi-asset transaction with Ada/"
Randomized with seed 947347928
Finished in 128.2986 seconds
173 examples, 2 failures, 7 pending
e11d935
to
3e4dfb7
Compare
let val = [(toText pid, toText an, minUTxOValue)] | ||
|
||
-- Allow for a higher minimum utxo coin due to assets | ||
let minAda = minUTxOValue * 2 |
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 should ultimately provide this as an API endpoint.
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.
Sounds good.
edcdb8b
to
a6f2d22
Compare
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.
Hi @rvl
Just some minor comments.
One thing I noticed is that a few functions accept or return multiple String
s in their type signatures, and it wasn't always obvious to me (without some digging) which String
referred to what. Perhaps it would be worth making some newtypes to replace the use of String
in a few places?
But overall looks good!
lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Transactions.hs
Outdated
Show resolved
Hide resolved
|
||
-- | A token bundle, along with the keys and scripts necessary to mint the | ||
-- bundle. | ||
type MintTokenBundle = (TokenBundle, [(String, String)]) |
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.
Perhaps it would be worth defining type aliases or even newtypes for the String
parts?
(Just to aid readability, as it might not be immediately clear which String
refers to which value.)
There are a few functions where these are passed around: I suspect it might help make the code easier to follow if these have named types.
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.
Yes good point - I would like to have made a record type for this.
But we have a package organisation issue that make it impractical to do at the moment.
I have tried to improve the situation a little by replacing one string with TokenPolicyId, and adding more haddock.
maryTokenBundles :: [MintTokenBundle] | ||
maryTokenBundles = tokenBundles maryAssetScripts | ||
|
||
tokenBundles :: [(String, (String, String))] -> [MintTokenBundle] |
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.
Same here. (regarding String
)
r <- request @ApiWallet ctx (Link.getWallet @'Shelley w) Default Empty | ||
verify r | ||
[ expectField (#assets . #available) (`shouldNotBe` mempty) | ||
, expectField (#assets . #total) (`shouldNotBe` mempty) |
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.
Just a suggestion, but perhaps TokenBundle.empty
here might make this more explicit. (And at other similar places where mempty
is used.)
(I'm guessing that it's a TokenBundle
😃)
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 type of it is not too important ... "some kind of assets".
I applied your suggestion - see if it looks better or worse.
=> Context | ||
-> ApiWallet | ||
-> Natural | ||
-> [((Text, Text), Natural)] |
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.
Perhaps it would be nice to add a field comment here or type alias here, just to state what these Text
values should be.)
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.
There is a comment which says (PolicyId Hex, AssetName Hex)
just above - is that alriight?
build (InsufficientMinCoinValueError o c) = unlinesF | ||
[ nameF "Expected min coin value" (build c) | ||
, nameF "TxOut" (build o) | ||
] |
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.
👍🏻
1673496
to
65cc0d8
Compare
verify r | ||
[ expectField (#assets . #available . #getApiT) (`shouldNotBe` TokenMap.empty) | ||
, expectField (#assets . #total . #getApiT) (`shouldNotBe` TokenMap.empty) | ||
] |
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.
👍
(`shouldNotBe` TokenMap.empty) | ||
, expectField (#balance . #total . #getQuantity) | ||
(`shouldBe` minCoin) | ||
] |
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.
👍
rtx <- request @(ApiTransaction n) ctx | ||
(Link.createTransaction @'Shelley wSrc) Default payload | ||
-- It should fail with InsufficientMinCoinValueError | ||
expectResponseCode HTTP.status403 rtx |
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.
👍
verify rb | ||
[ expectField (#assets . #available . #getApiT) (`shouldNotBe` TokenMap.empty) | ||
, expectField (#assets . #total . #getApiT) (`shouldNotBe` TokenMap.empty) | ||
] |
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.
👍
verify rb | ||
[ expectField (#assets . #available . #getApiT) (`shouldNotBe` TokenMap.empty) | ||
, expectField (#assets . #total . #getApiT) (`shouldNotBe` TokenMap.empty) | ||
] |
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.
👍
65cc0d8
to
8cb42ae
Compare
bors merge |
*please |
2465: integration tests: Mint assets in the cluster faucet, and add basic MA scenarios r=KtorZ a=rvl ### Issue Number ADP-604 ### Overview - Faucet mints assets as part of allocating funds to wallets. - There are 3 pre-generated policy objects in faucet. - Add a new type of fixture wallet for multi-asset. - Add a test for the assets balance of a fixture wallet. - Add a pending test to transfer assets between wallets. ### Comments - `cardano-cli --tx-out` syntax described here: [cli TxOutParser](https:/input-output-hk/cardano-node/blob/master/cardano-cli/src/Cardano/CLI/Mary/TxOutParser.hs) and [ValueParser](https:/input-output-hk/cardano-node/blob/master/cardano-cli/src/Cardano/CLI/Mary/ValueParser.hs). - Rather than assigning assets to faucet wallets, integration tests could also submit asset minting transactions with cardano-cli as part of setup. - Setup code is a little bit scrappy, sorry. Co-authored-by: Rodney Lorrimar <[email protected]>
Build failed: There were two test cases which needed to be retried.
And the "real" failures which failed twice:
#expected |
Haven't seen that one before 🤔 |
Yes hmm |
8cb42ae
to
8ce1597
Compare
I think the tests failed because of a change I made which meant that some shelley faucet wallets were unfunded. bors r+ |
Build succeeded: |
Issue Number
ADP-695 and a little ADP-604.
Overview
Comments
cardano-cli --tx-out
syntax described here: cli TxOutParser and ValueParser.