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

Don't show TX error messages for empty error blocks #1444

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

csillag
Copy link
Contributor

@csillag csillag commented Jun 5, 2024

Some transactions have an error block like this:

"error": {
  "code": 0,
  "module": ""
}

An example found by @matevz is available here.

In these cases, no error message should be shown.

Copy link

github-actions bot commented Jun 5, 2024

Deployed to Cloudflare Pages

Latest commit: f7c66a24f729822878194e6292196b13a2d91eec
Status:✅ Deploy successful!
Preview URL: https://86772af1.oasis-explorer.pages.dev

@csillag
Copy link
Contributor Author

csillag commented Jun 5, 2024

This is marked as draft until someone can verify that my interpretation of the situation is correct.

@csillag
Copy link
Contributor Author

csillag commented Jun 5, 2024

This is marked as draft until someone can verify that my interpretation of the situation is correct.

Yes: @kostko says that

Yes code 0 is always success.

So, we don't have to show an error message here.

@csillag csillag marked this pull request as ready for review June 5, 2024 14:14
@@ -3,7 +3,7 @@ import { TxError } from '../../oasis-nexus/api'

export const useTxErrorMessage = (error: TxError | undefined): string | undefined => {
const { t } = useTranslation()
if (!error) return undefined
if (!error || (!error.module && !error.code)) return undefined
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why check for a module? I think simplification should suffice:

Suggested change
if (!error || (!error.module && !error.code)) return undefined
if (!error?.code) return undefined

Copy link
Contributor Author

@csillag csillag Jun 5, 2024

Choose a reason for hiding this comment

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

Why check for a module?

Because I don't know if we can assume that all possible modules (i.e. sources of errors) are sane enough to use the 0 error code for success.

There might be some deranged modules that break the convention.

Generally speaking, when hiding error messages, I want to be as narrow and specific as possible. We know that no module and error code 0 is not a problem, so we can hide this, but nothing else.

If we see more useless error messages later, we can also hide them later...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a TODO, if this ever gets resolved upstream.

@lukaw3d
Copy link
Member

lukaw3d commented Jun 5, 2024

Frontend should not be fixing this

@lubej
Copy link
Collaborator

lubej commented Jun 6, 2024

Frontend should not be fixing this

How about we create an issue upstream?
And merge this quick fix for now. As it takes "ages" to propagate the change on the BE side to mainnet.

@csillag
Copy link
Contributor Author

csillag commented Jun 6, 2024

How about we create an issue upstream?

The discussion with the Nexus guys have been going on for 3 days now.

And merge this quick fix for now.

I agree, but waiting for @lukaw3d's decision.

As it takes "ages" to propagate the change on the BE side to mainnet.

Exactly.

@lukaw3d
Copy link
Member

lukaw3d commented Jun 6, 2024

🤷 seems very rare, and frontend can't correctly fix the transaction anyway: method missing, and status missing

@csillag
Copy link
Contributor Author

csillag commented Jun 28, 2024

Not much progress with the Nexus issue for the last 3 weeks. I still thing we ought to merge the frontend workaround, at least in order to remove the spurious error message...

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.

None yet

3 participants