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

Sub 1s loading UI #169

Merged
merged 27 commits into from Feb 26, 2024
Merged

Sub 1s loading UI #169

merged 27 commits into from Feb 26, 2024

Conversation

Keyrxng
Copy link
Contributor

@Keyrxng Keyrxng commented Feb 20, 2024

Resolves #168

The issue with things as far as I could see was that the UI rendering was being held back due to the laggy RPC calls and you'd see this:

hold_my_brewery.mp4

Although using the static data available through the permit and hardcoded token info we can see this instead:

hold_my_beer.mp4

It has involved moving around a few parts but the new interval for the loading ui rendering is sub 1s

new_load_time

With a forked foundry instance, I was trying to replicate the stalling offline to see if I could squeeze a tx through while the stalling rpc calls takes its time but I'm not able to replicate it and work with real permits as local anvil hits instantly everytime.

But thinking about it, you should be able to as mm will be using a different rpc/provider instance so long as the it's not locking down the UI and a user can click they should be able to make a claim with or without the treasuryInfo

@0x4007
Copy link
Member

0x4007 commented Feb 20, 2024

Your code is outdated. Pull from head

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 20, 2024

lmao I'll get there sooner or later with Git 😂

lib/chainlist Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you're up to date? If you are then you might need to rebase or something because its unlikely that you went out of your way to remove this module.

@ubiquibot-continuous-deploys
Copy link

ubiquibot-continuous-deploys bot commented Feb 21, 2024

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 21, 2024

Optimistic rendering time till claimable:

optimistic-w-add-rpcs

Non-optimistic rendering time till claimable:

non-optimistic-w-add-rpcs

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 21, 2024

had a nightmare trying to get that submodule added, thats what caused my initial issues it was crashing my gh desktop app and would not pull/update or replace because it existed in the index no matter how many time I removed it from cache and the git file, nightmare but got there in the end

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 21, 2024

The rpc speed mapping is cool but I think it's a bit overkill pinging that many rpcs just to fetch a balance and allowance lmao especially if it's a mainnet tx, hitting 49 RPCs is nuts, 15 for gnosis is still pretty wild

Looking at the privacy statements within extraRpcs.js some of them look pretty scary with all of the data that are collecting esp when you consider the maxi mindset

A task could be to read through them and exclude specific rpcs and lean towards those that collect no data or only onchain data and use the extraRPCs in other areas with direct on-chain writes as opposed to just in this case two reads

@rndquu
Copy link
Member

rndquu commented Feb 21, 2024

local anvil hits instantly everytime

As far as I remember when you use a forked anvil instance and send a transaction then it is not mined instantly but waits for a new block from a forked network. Anyway there is also this option since anvil supports hardhat's RPC requiest.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 21, 2024

It's not determined by a block being mined as it involves no onchain writes, it's hitting 127.0.0.1:8545 with a read op to getChainId, followed by two reads for balance and allowance. Afaik you can't stop anvil/hardhat from responding to reads but you could obv cause a write op to 'stall' if you disable auto mine

@rndquu
Copy link
Member

rndquu commented Feb 21, 2024

It's not determined by a block being mined as it involves no onchain writes, it's hitting 127.0.0.1:8545 with a read op to getChainId, followed by two reads for balance and allowance. Afaik you can't stop anvil/hardhat from responding to reads but you could obv cause a write op to 'stall' if you disable auto mine

I see, there is also the --no-storage-caching option which will add some delay to read operations but obviously it would be better to have a separate delay option for onchain read operations

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 21, 2024

It's not determined by a block being mined as it involves no onchain writes, it's hitting 127.0.0.1:8545 with a read op to getChainId, followed by two reads for balance and allowance. Afaik you can't stop anvil/hardhat from responding to reads but you could obv cause a write op to 'stall' if you disable auto mine

I see, there is also the --no-storage-caching option which will add some delay to read operations but obviously it would be better to have a separate delay option for onchain read operations

I just started looking over some of the flags, often updating and changing plus I've forgotten most I think lmao but that might help test the edge case but either way the UI currently loads quicker than the metamask popup so trying to squeeze a tx through while bal and allowance and fetching is very unlikely and if already connected via mm before page load it still shouldn't interfere with the fetching as its two distinct providers for reads and writes

@0x4007
Copy link
Member

0x4007 commented Feb 21, 2024

I got it under one second on my branch. Just fixing some details around refactoring the code

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 21, 2024

As did I without the additional 15 rpc calls however it's a good fallback function to have if writes are timing out or whatever but for two reads it's heavy artillery 😂

@0x4007
Copy link
Member

0x4007 commented Feb 22, 2024

As did I without the additional 15 rpc calls however it's a good fallback function to have if writes are timing out or whatever but for two reads it's heavy artillery 😂

My plan is to combine both. Unfortunately I did a large refactor here (again) breaking apart large functions into seperate files. I anticipate some more merge conflicts. Let me get mine merged in first and then we can merge yours in?

@0x4007
Copy link
Member

0x4007 commented Feb 22, 2024

I was going to merge yours in but I realized that its not stable either due to a combination of my bad commit and the refactoring being incompatible with some pulls getting merged at the same time. Can you get this stable so we can use it for production? I need more time to fix my branch stability.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 23, 2024

I'm a little confused as to what you mean by it's not currently stable. I'm seeing no errors and was able to successfully claim on 29a1da8 , the UI near instantly loads and there are no conflicts

I realise that I'm merging against ubiquity:main and not into ubiquity:development might that be part of the issue? lmk what the best thing to do would be whether to merge your refactor branch into here, vice-versa or whether to open a new pr against development, cheers

image

@0x4007
Copy link
Member

0x4007 commented Feb 23, 2024

I'm a little confused as to what you mean by it's not currently stable. I'm seeing no errors and was able to successfully claim on 29a1da8 , the UI near instantly loads and there are no conflicts

It seems unstable when I refresh several times and press claim. If you set up a test wallet you can generate real permits with you can test almost the entire flow up until you confirm the transaction. You will see it only works some times. I assume there is an issue with some of the RPCs that need to be taken into account.

I realise that I'm merging against ubiquity:main and not into ubiquity:development might that be part of the issue?

I don't think so but you should merge to development. It was my mistake because I briefly set main as the default branch but I realize given the pace of progress, stability might not be 100% so I switched the default back to development. After manual testing we can merge to main which automatically deploys to pay.ubq.fi

lmk what the best thing to do would be whether to merge your refactor branch into here, vice-versa or whether to open a new pr against development, cheers

You just hit edit on the top right and change the merge target to development. Either way though the intermittent issues I mentioned I saw on your latest commit.

image

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 23, 2024

Hitting localhost I generated 10 permits with an open window each, refreshed a few times on each and claimed to completion and all I could see was a little layout shift to the right within the claim box.

Using the networkRPCs the claim button flickers when an RPC call fails, specifically "https://xdai-archive.blockscout.com" so when that's blacklisted the claim button stops flickering but the layout shift is still there

@0x4007
Copy link
Member

0x4007 commented Feb 23, 2024

Let me spend some time testing today and I'll be careful to clear cache etc to ensure that everything works as expected.

I'll see if I can post proof of the specific error I remember receiving.

Thanks for testing!

const officialUrls = extraRpcs[networkId].rpcs.filter((rpc) => {
if (typeof rpc === "string") {
if (blacklist.includes(rpc)) {
return "";
Copy link
Member

Choose a reason for hiding this comment

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

That doesn't look right. You should not return a truthy value if you want to filter it out.

Comment on lines 74 to 80
export async function generateMultiERC20Permits() {
for (let i = 0; i < 5; i++) {
const url = await generate(true);
log.ok("Testing URL:");
console.log(url);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to put this in a local test file?

@0x4007
Copy link
Member

0x4007 commented Feb 25, 2024

The error that I got is from.

erc20-permit.ts:67 Error: could not detect network (event="noNetwork", code=NETWORK_ERROR, version=providers/5.7.2)
image

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 26, 2024

This resolves the no network issue.

I test by setting the CHAIN_ID in .env to 31337 and then forking gnosis using flag --chain-id 31337 and then when testing against the actual RPCs I do the same but setting the chain ids to 100

@@ -14,13 +14,13 @@ export const entries = [...typescriptEntries, ...cssEntries];
const allNetworkUrls: Record<string, string[]> = {};
// this flattens all the rpcs into a single object, with key names that match the networkIds. The arrays are just of URLs per network ID.

const blacklist = ["https://xdai-archive.blockscout.com"];
const blacklist = ["https://xdai-archive.blockscout.com", "https://gnosis.api.onfinality.io/public"];
Copy link
Member

Choose a reason for hiding this comment

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

This blacklist idea doesn't seem like a robust approach. We can merge it in but it should be handled dynamically. I think I implemented this in an unstable manner on my branch, I vaguely recall.

Copy link
Contributor Author

@Keyrxng Keyrxng Feb 26, 2024

Choose a reason for hiding this comment

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

Since the issue is that the claim button flickers on a failure then finding out why exactly that's happening because of a failed background call would probs be the best way to handle it I guess, otherwise just try{}catch() and don't log the error? seems hacky we could care less if one or two calls have failed

Copy link
Member

@0x4007 0x4007 Feb 26, 2024

Choose a reason for hiding this comment

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

It's because the CSS/style logic is implemented poorly. I tore it apart in my refactor but didn't get to put it all together in a totally stable way (mostly the other RPC related functions- I believe the styling code was significantly simplified and stable if I recall correctly).

Unfortunately I'm running very low on code time because I'll need to focus on fundraising starting next week. So I'm just going to be focusing on top priority stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Luckily for me I've got nothing but code time, regretfully it does eventually require that you have review time lmao but I'll keep on pushing forward with things as best I can in the meantime, cheers for the heads up

@0x4007 0x4007 merged commit 123f178 into ubiquity:main Feb 26, 2024
1 check passed
@@ -33,8 +33,7 @@ export async function handleNetwork(desiredNetworkId: number) {
invalidateButton.disabled = true;
}

const network = await web3provider.getNetwork();
const currentNetworkId = network.chainId;
const currentNetworkId = (await web3provider.getNetwork()).chainId;
Copy link
Member

Choose a reason for hiding this comment

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

Why did you recombine this?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad I thought I had replied to this, it was not intentional I can only assume it was from my merging of branches and it's been an oversight to spreading it out again

await insertErc20PermitTableData(permit, provider, symbol, decimals, table);
}

export async function fetchTreasury(contractAddr: string, owner: string, provider: JsonRpcProvider) {
Copy link
Member

Choose a reason for hiding this comment

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

More typesafe to define the return type up here via

export async function fetchTreasury(contractAddr: string, owner: string, provider: JsonRpcProvider): { balance: BigNumber; allowance: BigNumber } {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll catch this and the rest in the sync ERC pr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something I wasn't able to produce before, spamming refresh there are a few failed calls maybe 3 or 4 and the balance and allowance render as 0, I'll see if I can get that too

@ubiquibot ubiquibot bot mentioned this pull request Feb 26, 2024
0x4007 added a commit to 0x4007/pay.ubq.fi that referenced this pull request Mar 9, 2024
…ements"

This reverts commit 123f178, reversing
changes made to 7d88536.
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.

Reduce loading time
5 participants