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 generated cbor error reason in transaction submit testing #90

Draft
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

ginnun
Copy link
Collaborator

@ginnun ginnun commented Nov 25, 2024

Context

This is far from being ready to review.

  • json file format has changed, so i need to update my part
  • i need to fix obvious problems

Important Changes Introduced

Copy link
Member

@michalrus michalrus 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 dropping the 2 first bytes from the test cases. :) And for logging all CBORs, too, not only the ones that failed to deserialize. A few comments:

#[cbor(transparent)]
pub struct DisplayCoin(#[n(0)] Coin);

impl fmt::Display for DisplayCoin {
Copy link
Member

Choose a reason for hiding this comment

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

I don’t know if it wouldn’t be cleaner to define a new trait, say, HaskellDisplay, and then you could define it for pallas_primitives::Coin, without a need for a wrapper.

It would be more natural to use for users of this serializing library, if it ever becomes one.

But in terms of runtime, this newtype-style wrapper is zero-cost, so fine.

_haskell_repr: String,
json_naive: Vec<String>,
#[serde(rename(deserialize = "jsonSubmitAPI"))]
_json_submit_api: String,
Copy link
Member

Choose a reason for hiding this comment

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

You know that already, but I changed the format – sorry! Now there's only { cbor, haskellRepr, json, typeTag }, e.g.

{
  "cbor": "818206828201820083051a000151351a00074b8582076162",
  "haskellRepr": "ApplyTxError (ConwayUtxowFailure (UtxoFailure (FeeTooSmallUTxO (Coin 86325) (Coin 478085))) :| [ConwayMempoolFailure \"b\"])",
  "json": {
    "contents": {
      "contents": {
        "contents": {
          "era": "ShelleyBasedEraConway",
          "error": [
            "ConwayUtxowFailure (UtxoFailure (FeeTooSmallUTxO (Coin 86325) (Coin 478085)))",
            "ConwayMempoolFailure \"b\""
          ],
          "kind": "ShelleyTxValidationError"
        },
        "tag": "TxValidationErrorInCardanoMode"
      },
      "tag": "TxCmdTxSubmitValidationError"
    },
    "tag": "TxSubmitFail"
  },
  "typeTag": "ApplyTxError (ConwayEra StandardCrypto)"
}

But you know that already. :)

#[serde(rename_all(deserialize = "camelCase"))]
pub struct CborTestCases {
_seed: u64,
_type_tag: String,
Copy link
Member

Choose a reason for hiding this comment

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

{ typeTag } is gone from this structure.

@@ -72,6 +81,26 @@ impl NodeClient {
}
}

pub fn try_decode_error(buffer: &[u8]) -> Result<TxValidationError, Error> {
let maybe_error = Decoder::new(buffer).decode();
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for removing the dropping of the two first bytes (&buffer[2..]) from here!

But it's nowhere to be found now. 👀 I think you forgot to add it near transaction submission:

diff --git a/src/node/transactions.rs b/src/node/transactions.rs
index b85124b..303078f 100644
--- a/src/node/transactions.rs
+++ b/src/node/transactions.rs
@@ -45,7 +45,7 @@ impl NodeClient {
                 Ok(txid)
             }
             Ok(Response::Rejected(reason)) => {
-                let reason = reason.0;
+                let reason = &reason.0[2..];
                 let msg_res = Self::try_decode_error(&reason);

                 match msg_res {

Copy link
Member

Choose a reason for hiding this comment

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

No longer relevant after merging main into this.

Copy link

cloudflare-workers-and-pages bot commented Nov 29, 2024

Deploying blockfrost-platform with  Cloudflare Pages  Cloudflare Pages

Latest commit: a5c74c8
Status: ✅  Deploy successful!
Preview URL: https://a526a4ba.blockfrost-platform.pages.dev
Branch Preview URL: https://feat-submit-error-testing.blockfrost-platform.pages.dev

View logs

@@ -0,0 +1,26 @@
{
"seed": 1732614703134,
Copy link
Member

Choose a reason for hiding this comment

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

@ginnun I was thinking… Since we already have testgen-hs available under pub fn crate::cbor::fallback_decoder::FallbackDecoder::locate_child_binary(), and it’s crucial for the system to work…

… what would you say instead of committing the JSON fixtures, that we only saved the seeds and generated the JSONs with the command each time? Passing --seed would make it stable across runs.

My reasoning was that if devs in cardano-node change something, our tests will detect that change. But static JSON files won’t. 🤔

I can prepare a branch on top of this one, if you'd like to test this idea. I'll need to prepare something similar in the future for the random QuickCheck-style property testing of the deserializer, after you get it all done. :)

@ginnun ginnun force-pushed the feat/submit-error-testing branch from e895ff7 to 06c16c3 Compare January 20, 2025 13:22
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.

2 participants