Add support for AwaitTxStatusChangeReq contract request #83

nazrhom · 2022-03-31T14:56:54Z

Change-type: minor
Signed-off-by: Giovanni Garufi giovanni@mlabs.city

nazrhom · 2022-03-31T15:00:04Z

test/Spec/MockContract.hs

      case drop 1 $ dropWhile (/= "--out-file") args of
        filepath : _ ->
-          modify @(MockContractState w) (files . at (Text.unpack filepath) ?~ OtherFile "TxBody")
+          modify @(MockContractState w) (files . at (Text.unpack filepath) ?~ _)


Tests are currently failing as we didn't previously care for the contents of these files but now we want to read them back to return the Tx

@gege251 do you have any idea what the best solution for this could be? Atm mockReadFileTextEnvelope fails because these are stored as OtherFile but to store them as TextEnvelopeFiles I need to serialise an actual tx into it

A bit hacky idea, but what if we would create a new TxFile constructor for MockFile, and use that in the pattern match in mockReadFileTextEnvelope, and return some dummy CardanoTx with some random id (I think, we don't need it to be correct at this point).

this might be easier said that done.

nazrhom · 2022-03-31T15:00:44Z

src/BotPlutusInterface/Contract.hs

          , "Signatories (pkh): " <> Text.unwords (map pkhToText requiredSigners)
          ]

+    let ext = if signable then "signed" else "raw"


Is this even correct? Seems to be the case from the tests but I am not sure we will always have one of the two files

"raw" must be there in each case, "signed" is only available, if we have all the necessarry signing keys to sign the tx. I think this works.
Finish without sign and submit behaviour is not exactly ideal, and we had a few discussions about this with Sam, but didn't really have time to improve it unfortunately...
[nits] It's not at all important, but I think we could move this part (reading the actual tx and txId from file and changing the filenames) between the build and sign steps, so we don't have to do all these conditionals.

Change-type: minor Signed-off-by: Giovanni Garufi <giovanni@mlabs.city>

samuelWilliams99

The way we're handling this doesn't give a safe path back to regular execution if a tx doesn't make it.
We need to return a non valid tx response if we don't ever see the transaction on the index, or if it gets rolled back, otherwise we can't really handle failure here.
The question of how long to wait before assuming a tx didn't land is an annoying one, as we cant be fully sure unless we enforce a validity range on a tx.
So, in this case, lets do that.
If the tx has a validity range, and we shoot past it without seeing it in the block, return out. otherwise, have some hard coded long delay before assuming failure.

Almost fix tests with a hacky solution

nazrhom · 2022-04-01T13:27:00Z

@samuelWilliams99 I added a timeout of 5 blocks after which the the status is reported as Unknown (might be a bit too short, running local chain-index I always see it popping up after 1/2 blocks) if we don't see the tx in chain-index. Im not sure about the validity range in this context. From what I understood we will only ever see the Tx on chain-index if it is phase-1 valid so if we every get a response back from there, I think I should be able to assume that we are in a valid interval for that Tx.

…the Tx does not show up in chain-index after a set number of blocks (currently 5) Change-type: patch Signed-off-by: Giovanni Garufi <giovanni@mlabs.city>

samuelWilliams99 · 2022-04-02T17:29:54Z

I interval thing was a way of knowing definitively if we've waited enough. With our current solution, we're assuming all txs will land within some time frame, but if we use validity interval, we know theres a point at which a tx MUST have failed, and isn't just caught up in traffic

…-index response to the contract Change-type: patch Signed-off-by: Giovanni Garufi <giovanni@mlabs.city>

samuelWilliams99

since the txId was wrong, does this mean that the new txid we'll get back from submitting a transaction won't work with the raw tx endpoint?

samuelWilliams99 · 2022-04-04T19:51:14Z

src/BotPlutusInterface/Helpers.hs

+        Unknown -> do
+          curSlot <- currentSlot
+          if curSlot >= maxSlot
+            then throwError @e $ review _OtherContractError $ pack $ "Could not find tx after maxAttempts. ID: " ++ show i


this isn't really max attempts anymore, so this error is misleading

samuelWilliams99 · 2022-04-04T19:51:33Z

src/BotPlutusInterface/Helpers.hs

+import Prelude
+
+awaitTxConfirmedUntilSlot :: forall w s e. (AsContractError e) => TxId -> Slot -> Contract w s e ()
+awaitTxConfirmedUntilSlot i maxSlot = go 0


not a fan of i, as thats usually a loop integer. perhaps txId or something

samuelWilliams99 · 2022-04-04T19:52:29Z

src/BotPlutusInterface/Helpers.hs

+      case mTx of
+        Unknown -> do
+          curSlot <- currentSlot
+          if curSlot >= maxSlot


lets have maxslot as inclusive, as thats what maximum implies, so switch this to a >

samuelWilliams99 · 2022-04-04T19:53:21Z

test/Spec/MockContract.hs

-    -- pure $ TxIdResponse Nothing
-    throwError @Text "TxFromTxId is unimplemented"
+  TxFromTxId txId -> do
+    -- TODO: Track some kind of state here, add tests to ensure this works correctly


Get a ticket for this if one doesnt already exist

… tx id Change-type: minor Signed-off-by: Giovanni Garufi <giovanni@mlabs.city>

nazrhom · 2022-04-05T00:43:37Z

since the txId was wrong, does this mean that the new txid we'll get back from submitting a transaction won't work with the raw tx endpoint?

Unfortunately you are correct. I added an extra commit that renames the files with the correct tx id... but it feels wrong :P
We are piling a lot of hacks on top of another one here; it's possibly worth to get other things unblocked, but we should make it a priority to not leave this in this state too long.

szg251

I only had a few nits to comment, looks good to me!

szg251 · 2022-04-05T08:17:37Z

src/BotPlutusInterface/CardanoCLI.hs

 txOutRefToCliArg :: TxOutRef -> Text
-txOutRefToCliArg (TxOutRef (TxId txId) txIx) =
-  encodeByteString (fromBuiltin txId) <> "#" <> showText txIx
+txOutRefToCliArg (TxOutRef (TxId tId) txIx) =


[nits] I think there's a typo here

Had to rename that because we are importing txId from Ledger.Tx

szg251 · 2022-04-05T08:29:49Z

src/BotPlutusInterface/Contract.hs

          , "Signatories (pkh): " <> Text.unwords (map pkhToText requiredSigners)
          ]

+    let ext = if signable then "signed" else "raw"


"raw" must be there in each case, "signed" is only available, if we have all the necessarry signing keys to sign the tx. I think this works.
Finish without sign and submit behaviour is not exactly ideal, and we had a few discussions about this with Sam, but didn't really have time to improve it unfortunately...
[nits] It's not at all important, but I think we could move this part (reading the actual tx and txId from file and changing the filenames) between the build and sign steps, so we don't have to do all these conditionals.

szg251 · 2022-04-05T08:46:26Z

test/Spec/BotPlutusInterface/Contract.hs

        submitTx constraints

-  assertContractWithTxId contract initState $ \state outTxId ->
+  assertContractWithTxId contract initState $ \state _ ->


[nits] We don't need to use assertContractWithTxId anymore, this could be replaced with a simple runContractPure

szg251 · 2022-04-05T08:51:24Z

test/Spec/BotPlutusInterface/Contract.hs

              Constraints.mustPayToPubKey paymentPkh2 (Ada.lovelaceValueOf 1000)
        txId <- submitTx constraints
-        tell $ Last $ Just $ Text.pack $ show $ Tx.txId <$> txId
+        tell $ Last $ Just test


[nits] The purpose of returning the txId was to prove that we can return values from the contract, but I think it is not really important to include in this test case.
We could just assert that the value in the observable state is "Init contract".

Change-type: patch Signed-off-by: Giovanni Garufi <giovanni@mlabs.city>

nazrhom force-pushed the nazrhom/await-tx-status-change branch from fa8003e to 0d871e4 Compare March 31, 2022 14:58

nazrhom commented Mar 31, 2022

View reviewed changes

nazrhom requested review from samuelWilliams99 and szg251 and removed request for samuelWilliams99 March 31, 2022 15:01

Add support for AwaitTxStatusChangeReq contract request

d2e3e87

Change-type: minor Signed-off-by: Giovanni Garufi <giovanni@mlabs.city>

nazrhom force-pushed the nazrhom/await-tx-status-change branch from 0d871e4 to d2e3e87 Compare March 31, 2022 15:21

samuelWilliams99 reviewed Mar 31, 2022

View reviewed changes

szg251 and others added 2 commits April 1, 2022 13:01

Almost fix tests with a hacky solution

ced427e

Merge pull request #84 from mlabs-haskell/gergely/hack-tests

86f19b6

Almost fix tests with a hacky solution

nazrhom force-pushed the nazrhom/await-tx-status-change branch 3 times, most recently from 7768b6c to 342f7a2 Compare April 1, 2022 15:27

Fix tests and update awaitTxStatusChange so that it can also fail if …

fd77f79

…the Tx does not show up in chain-index after a set number of blocks (currently 5) Change-type: patch Signed-off-by: Giovanni Garufi <giovanni@mlabs.city>

nazrhom force-pushed the nazrhom/await-tx-status-change branch from 342f7a2 to fd77f79 Compare April 1, 2022 16:46

nazrhom requested a review from samuelWilliams99 April 4, 2022 09:24

nazrhom added 2 commits April 4, 2022 17:54

Add awaitTxConfirmedUntilSlot and move the initial wait for the chain…

885372f

…-index response to the contract Change-type: patch Signed-off-by: Giovanni Garufi <giovanni@mlabs.city>

Merge branch 'master' into nazrhom/await-tx-status-change

d68c6a4

samuelWilliams99 reviewed Apr 4, 2022

View reviewed changes

nazrhom force-pushed the nazrhom/await-tx-status-change branch from a968353 to 0eb5051 Compare April 4, 2022 23:14

Cleanup old code

bd6421f

nazrhom force-pushed the nazrhom/await-tx-status-change branch from 0eb5051 to bd6421f Compare April 5, 2022 00:36

Update the raw and signed files generated by cardano-cli with the new…

26ea424

… tx id Change-type: minor Signed-off-by: Giovanni Garufi <giovanni@mlabs.city>

szg251 approved these changes Apr 5, 2022

View reviewed changes

Update according to revie

1b5e68a

Change-type: patch Signed-off-by: Giovanni Garufi <giovanni@mlabs.city>

Always query the raw transaction file in writeBalanceTx

1caede2

Change-type: patch Signed-off-by: Giovanni Garufi <giovanni@mlabs.city>

samuelWilliams99 approved these changes Apr 5, 2022

View reviewed changes

Update plutus-game with correct ExBudget

ff9db92

Change-type: patch Signed-off-by: Giovanni Garufi <giovanni@mlabs.city>

samuelWilliams99 approved these changes Apr 5, 2022

View reviewed changes

nazrhom merged commit c1918de into master Apr 5, 2022

szg251 deleted the nazrhom/await-tx-status-change branch July 20, 2022 06:53

Add support for AwaitTxStatusChangeReq contract request #83

Add support for AwaitTxStatusChangeReq contract request #83

Uh oh!

Conversation

nazrhom commented Mar 31, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nazrhom Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szg251 Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samuelWilliams99 left a comment

Choose a reason for hiding this comment

Uh oh!

nazrhom commented Apr 1, 2022

Uh oh!

samuelWilliams99 commented Apr 2, 2022

Uh oh!

samuelWilliams99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nazrhom commented Apr 5, 2022

Uh oh!

szg251 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nazrhom Mar 31, 2022 •

edited

Loading

szg251 Apr 1, 2022 •

edited

Loading