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

Feat: Descriptive Error Message #23

Merged
merged 3 commits into from
Apr 12, 2023
Merged

Feat: Descriptive Error Message #23

merged 3 commits into from
Apr 12, 2023

Conversation

Steveantor
Copy link
Contributor

Resolves #21

static/index.html Outdated Show resolved Hide resolved
static/index.html Show resolved Hide resolved
static/scripts/pay.ts Outdated Show resolved Hide resolved
static/scripts/pay.ts Outdated Show resolved Hide resolved
@Steveantor Steveantor force-pushed the main branch 4 times, most recently from 636029d to ce0ccc4 Compare April 6, 2023 16:19
@0x4007
Copy link
Member

0x4007 commented Apr 10, 2023

Do you have a permit link I can test this with?

@Steveantor
Copy link
Contributor Author

Do you have a permit link I can test this with?

here's one
permit-url
@pavlovcik

@ubiquibot
Copy link

ubiquibot bot commented Apr 10, 2023

Available commands

- /assign: Assign the origin sender to the issue automatically.
        ex: /assign

- /wallet <WALLET_ADDRESS | ENS_NAME>: Register the hunter's wallet address.
	ex1: /wallet 0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266
	ex2: /wallet vitalik.eth

@Steveantor

@0x4007
Copy link
Member

0x4007 commented Apr 10, 2023

This notation is not clear to me. What is the "$800.0"

image

@Steveantor
Copy link
Contributor Author

This notation is not clear to me. What is the "$800.0"

image

balance and approval @pavlovcik

@ubiquibot
Copy link

ubiquibot bot commented Apr 10, 2023

Available commands

- /assign: Assign the origin sender to the issue automatically.
        ex: /assign

- /wallet <WALLET_ADDRESS | ENS_NAME>: Register the hunter's wallet address.
	ex1: /wallet 0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266
	ex2: /wallet vitalik.eth

@Steveantor

@0x4007
Copy link
Member

0x4007 commented Apr 10, 2023

I need to think about the design to convey this information clearly and concisely. I feel like this should be in the "more details" ( ? ) dropdown details section.

I do like your implementation here, but the original bounty was supposed to be simpler: writing in plain english the issue when the errors dump into the DOM.

@0x4007
Copy link
Member

0x4007 commented Apr 10, 2023

I mocked up a new design can you implement the following:

localhost_8080__claim=eyJwZXJtaXQiOnsicGVybWl0dGVkIjp7InRva2VuIjoiMHg2QjE3NTQ3NEU4OTA5NEM0NERhOThiOTU0RWVkZUFDNDk1MjcxZDBGIiwiYW1vdW50IjoiMTAwMDAwMDAwMDAwMDAwMDAwMCJ9LCJub25jZSI6IjY5MTg1MDU5NjQxMDM4OTUwODY4NDU5MTA2NTgyMzExODg0MjQ5MjM4NTkyMD

@Steveantor
Copy link
Contributor Author

I mocked up a new design can you implement the following:

localhost_8080__claim=eyJwZXJtaXQiOnsicGVybWl0dGVkIjp7InRva2VuIjoiMHg2QjE3NTQ3NEU4OTA5NEM0NERhOThiOTU0RWVkZUFDNDk1MjcxZDBGIiwiYW1vdW50IjoiMTAwMDAwMDAwMDAwMDAwMDAwMCJ9LCJub25jZSI6IjY5MTg1MDU5NjQxMDM4OTUwODY4NDU5MTA2NTgyMzExODg0MjQ5MjM4NTkyMD

okay, I'll do and updating u here @pavlovcik

@ubiquity ubiquity deleted a comment from ubiquibot bot Apr 10, 2023
@Steveantor
Copy link
Contributor Author

should I delete the treasury field above? @pavlovcik

@ubiquibot
Copy link

ubiquibot bot commented Apr 10, 2023

Available commands

- /assign: Assign the origin sender to the issue automatically.
        ex: /assign

- /wallet <WALLET_ADDRESS | ENS_NAME>: Register the hunter's wallet address.
	ex1: /wallet 0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266
	ex2: /wallet vitalik.eth

@Steveantor

@0x4007
Copy link
Member

0x4007 commented Apr 10, 2023

Well to clarify, we should still have the plain english error when the claim fails (like when the raw MetaMask JSON RPC error dumps in the red monospace text on the DOM.) Please show both e.g:

  • plain english error
  • raw JSON RPC MetaMask error

However what you already implemented here could be technically considered as a separate bounty.

Yes you should remove the treasury field.

@Steveantor
Copy link
Contributor Author

upped

@Steveantor Steveantor force-pushed the main branch 3 times, most recently from 9ab35ec to 80107d2 Compare April 11, 2023 10:12
@0x4007
Copy link
Member

0x4007 commented Apr 12, 2023

From https://github.com/ubiquity/pay.ubq.fi
 ! [rejected]        refs/pull/23/head -> Steveantor/main  (non-fast-forward)
failed to run git: exit status 1

Git won't let me test this lol maybe you can rebase to main head

@Steveantor
Copy link
Contributor Author

From https://github.com/ubiquity/pay.ubq.fi
 ! [rejected]        refs/pull/23/head -> Steveantor/main  (non-fast-forward)
failed to run git: exit status 1

Git won't let me test this lol maybe you can rebase to main head

rebased @pavlovcik

@0x4007
Copy link
Member

0x4007 commented Apr 12, 2023

Not sure why I'm getting the same error.

@Steveantor
Copy link
Contributor Author

Not sure why I'm getting the same error.

might be your git client having issues

@0x4007
Copy link
Member

0x4007 commented Apr 12, 2023

Looks good.


Strangely enough it crashes on Safari right at the function execution but technically that's not in scope of the bounty. But still concerning that there's inconsistencies with Safari and Chrome for a simple promise.

Screenshot 2023-04-12 at 20 17 20

[Error] Unhandled Promise Rejection: Error: missing provider (argument="provider", value=undefined, code=INVALID_ARGUMENT, version=providers/5.7.2)

	(anonymous function) (app.ts:8)

@0x4007 0x4007 merged commit 4f2238b into ubiquity:main Apr 12, 2023
@0x4007 0x4007 mentioned this pull request Apr 14, 2023
0x4007 pushed a commit to 0x4007/pay.ubq.fi that referenced this pull request Jul 19, 2024
use `eslint-plugin-filename-rules`
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.

Descriptive Error Message: Balance
2 participants