Skip to content

Conversation

@stevenj
Copy link
Collaborator

@stevenj stevenj commented Jan 6, 2025

Description

This adds a uniform type to hold comprehensive problem reports that can occur when processing certain data.
These are distinct from hard errors, and are intended to report comprehensive reports to the user about problems
detected while processing their data.

This is intended to replace the current mutable vec of strings used for such purposes.

As the intention of such report is primarily to report to end users over API's, the report can serialize to json for easy inclusion in HTTP API responses.

Please confirm the following checks

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream module

@stevenj stevenj added the enhancement New feature or request label Jan 6, 2025
Copy link
Collaborator

@apskhem apskhem left a comment

Choose a reason for hiding this comment

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

LGTM

@apskhem apskhem merged commit 4995282 into feat/catalyst-types Jan 6, 2025
15 of 16 checks passed
@apskhem apskhem deleted the feat/problem-report branch January 6, 2025 04:23
@stanislav-tkach
Copy link
Member

I think that for such changes it is beneficial to see an actual usage to better judge the ergonomics of the API. Currently for me it isn't clear how to use it in typical scenarios. Also I'm not sure if the interior mutability is really needed here. I think that if we want to use it in a concurrent environment then it would be better to have some kind of merge functionality for reports instead of thread/tasks adding entries in an arbitrary order.

@stevenj
Copy link
Collaborator Author

stevenj commented Jan 6, 2025

This is the use case, expressed as a user story:

"As a user, I want to see every single problem related to any on-chain transaction that I make, or any document that I try and submit to a decentralized system. I do not want to be forced to make multiple attempts to correct all my problems because I can only correct problems one at a time. I also want to know what is correct about the document/transactions i submit even if parts of them are problematic in some way."

@bkioshn
Copy link
Contributor

bkioshn commented Jan 6, 2025

Here is how it will look like when implemented

{"context":"CIP36 Registration Validation","report":[{"kind":{"type":"InvalidValue","field":"network tag of payment address","value":"Testnet","constraint":"expected Mainnet"},"context":"CIP36 payment address network does not match the network used"}]}

I think it look much more structure, the only downside is a bit time consume getting each field right

@stanislav-tkach
Copy link
Member

@stevenj I understand the motivation and agree with that, but I'm talking more about an actual application in the code.

@bkioshn Thanks! Is it already used in your branch? I will look into that later. I'm specifically interested in how the code is structured.

@stevenj
Copy link
Collaborator Author

stevenj commented Jan 6, 2025

The point about interior mutability is just so that we do not need to manually manage mutability across call boundaries, we can pass a report as a context, and it is always able to be updated.

The point about concurrency, is so that the type is inherently Send and we have no issues passing around Reports after they are created, nor do we have issues with copies of the reports consuming large memory resources, because copies are shallow and cheap.

It is not intended in this use case that they be updated concurrently, even though the chosen type does support it. Further, the order of the reports is not sensitive at all, so there would be no problem with using a parallel iterator (if required) and adding reports in an arbitrary order based on the paralelism. Although, again, there is no current requirement to do that, doing so would not cause any issues.

@stevenj
Copy link
Collaborator Author

stevenj commented Jan 6, 2025

The application in the code is to return these errors in endpoints, we already have such responses for CIP36 endpoints, although the new type will allow those responses to be more comprehensive and detailed than currently specified.
See: the openapi specification for the response to api/draft/cardano/registration/cip36 the errors field is currently just a list of strings, which is how "problems" were previously reported, however the new type will allow that to be a better structured response.

stevenj added a commit that referenced this pull request Jan 7, 2025
* feat: initial commit

* ci: register crate

* feat(rust): Add uniform problem report type (#140)

* feat(rust): Add uniform problem report type

* fix(docs): spelling

* fix(rust): catalyst-types ci build

* fix(rust): doc tests

* feat: hash and coversion

* feat: uuid

* refactor: rename

* chore: fmtfix

* chore: fmtfix again

* test: integration

* fix: uuid type

* feat(rust/catalyst-types): Add duplicate data error report (#141)

* feat(catalyst-types): add duplicate data error report

Signed-off-by: bkioshn <bkioshn@gmail.com>

* fix(catalyst-types): format

Signed-off-by: bkioshn <bkioshn@gmail.com>

* fix(catalyst-types): add duplicate field description and change name

Signed-off-by: bkioshn <bkioshn@gmail.com>

* Update rust/catalyst-types/src/problem_report.rs

---------

Signed-off-by: bkioshn <bkioshn@gmail.com>
Co-authored-by: Steven Johnson <stevenj@users.noreply.github.com>

* chore: fmtfix

* chore: rename module fixing lint

* fix: module name

* chore: lintfix rename

* feat: kid

* chore: fmtfix

* feat: improve type errors

* chore: fmtfix

* fix: cspell

* fix: error coverage

* fix: lint

* fix: test

* fix: minor

* feat: lintfix

* fix: comment

* fix: comment

* chore: fmtfix

* Update rust/catalyst-types/src/conversion.rs

Co-authored-by: bkioshn <35752733+bkioshn@users.noreply.github.com>

* docs: update

* chore: kiduri

* chore: displaydoc

* fix: add displaydoc to .dic

---------

Signed-off-by: bkioshn <bkioshn@gmail.com>
Co-authored-by: Steven Johnson <stevenj@users.noreply.github.com>
Co-authored-by: Steven Johnson <sakurainds@gmail.com>
Co-authored-by: bkioshn <35752733+bkioshn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants