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

Beta to dev #334

Merged
merged 187 commits into from
Oct 15, 2024
Merged

Beta to dev #334

merged 187 commits into from
Oct 15, 2024

Conversation

rndquu
Copy link
Member

@rndquu rndquu commented Oct 14, 2024

This PR merges gift cards minting feature into the development branch.

EresDev added 30 commits May 11, 2024 03:19
Old RPC was causing issues randomly when forked with anvil
@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Oct 14, 2024

@ubiquity ubiquity deleted a comment from github-actions bot Oct 14, 2024
@0x4007
Copy link
Member

0x4007 commented Oct 14, 2024

I'm working on merging beta in as well at #333 but please let me know if you have an idea on how to fix the stack traces in the UI. Its not debuggable for me.

I paused on it and am continuing work on the new RPC manager.

Copy link
Contributor

github-actions bot commented Oct 14, 2024

@rndquu
Copy link
Member Author

rndquu commented Oct 14, 2024

I'm working on merging beta in as well at #333 but please let me know if you have an idea on how to fix the stack traces in the UI. Its not debuggable for me.

I paused on it and am continuing work on the new RPC manager.

I don't fully understand what you mean by "fix the stack traces". I'm getting the following console output in this branch:
Screenshot 2024-10-14 at 14 54 32

@rndquu rndquu marked this pull request as ready for review October 14, 2024 11:58
@rndquu rndquu requested a review from Keyrxng as a code owner October 14, 2024 11:58
@rndquu
Copy link
Member Author

rndquu commented Oct 14, 2024

@0x4007

and am continuing work on the new RPC manager.

Can't we just use the metamask provider as a fallback option instead of our rpc-handler (if it misbehaves)?

Overall this PR can be merged, we'll keep solving issues one by one (if there're any).

I haven't tried the actual gift card claiming, but at least gift cards are displayed. I'll need to check cloudflare env variables later.

@rndquu rndquu requested a review from 0x4007 October 14, 2024 12:01
Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Code mostly seems fine but two concerns:

  1. Stack traces for debugging seem useless now that it points to compiled javascript. We need this fixed or this is not maintainable.
  2. ubiquity-dollar.html seems like an odd name choice. What about something more specific related to the cards?

const tokenDecimals = await tokenContract.decimals();

return {
permitted: {
token: process.env.PAYMENT_TOKEN_ADDRESS || "",
amount: ethers.utils.parseUnits(amount || "", tokenDecimals),
token: permitConfig.PAYMENT_TOKEN_ADDRESS || "",
Copy link
Member

Choose a reason for hiding this comment

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

Empty string looks wrong


export const permitAllowedChainIds = [1, 5, 10, 100, 31337];

export const ubiquityDollarAllowedChainIds = [1, 100, 31337];
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should bridge to optimism soon


function createToast(meaning: keyof typeof toaster.icons, text: string, timeout: number = 5000) {
if (meaning != "info") buttonController.hideLoader();
//if (meaning != "info") buttonController.hideLoader();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe these should be removed

return;
}

transactionHistory.innerHTML = `
Copy link
Member

Choose a reason for hiding this comment

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

Seems like another opportunity to do that String.raw syntax and then html`

}

function transactionRowHtml(transaction: Transaction) {
return `
Copy link
Member

Choose a reason for hiding this comment

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

String.raw here as well

@rndquu rndquu marked this pull request as draft October 15, 2024 07:20
@rndquu rndquu marked this pull request as ready for review October 15, 2024 07:25
@0x4007
Copy link
Member

0x4007 commented Oct 15, 2024

I'm working on merging beta in as well at #333 but please let me know if you have an idea on how to fix the stack traces in the UI. Its not debuggable for me.

I paused on it and am continuing work on the new RPC manager.

I don't fully understand what you mean by "fix the stack traces". I'm getting the following console output in this branch:

Screenshot 2024-10-14 at 14 54 32

I missed this comment. Your logs seem fine! Mine were not pointing to the typescript source code.

@rndquu
Copy link
Member Author

rndquu commented Oct 15, 2024

@0x4007 Resolving code conflicts in this PR is a hell (especially when new PRs are merged regularly)

Can we merge this PR? I'll address all of the issues you stated in a separate one.

@0x4007 0x4007 merged commit 84c49d8 into ubiquity:development Oct 15, 2024
3 checks passed
@rndquu rndquu deleted the refactor/beta-to-dev branch October 16, 2024 13:57
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.

5 participants