Feature: Show transaction hashes for claimed permits #173

barebind · 2024-02-22T07:17:09Z

Resolves #138

ubiquity-os-deployer · 2024-02-22T07:18:22Z

73b9799

b6aa5b4

b800e77

959bbf7

0x4007 · 2024-02-22T09:19:32Z

73b9799

I'm working on a runtime fix at my branch now.

0x4007 · 2024-02-26T10:31:50Z

I think you'll need to pull from development because the app doesn't run. Did you test this? Also I fixed the continuous deploys within the time that you opened this pull request. If you push again it should deploy here and we will know definitively if its an environment issue etc.

Uncaught TypeError: (define_extraRpcs_default[1] || []) is not iterable
    at constants.ts:46:95
    at index.ts:20:52

static/scripts/rewards/web3/erc20-permit.ts

…imed-permits

barebind · 2024-02-27T05:51:00Z

I think you'll need to pull from development because the app doesn't run.

done.

rndquu · 2024-02-27T08:37:19Z

@barebind This PR is ready for review, right?

barebind · 2024-02-27T08:45:44Z

@barebind This PR is ready for review, right?

yes, I'm looking for adding database functions on my local supabase instance but getting a weird error. in any case there won't be any change on code side as it handles errors already. so yes, pr ready for review.

barebind · 2024-02-27T09:19:43Z

Uncaught TypeError: (define_extraRpcs_default[1] || []) is not iterable
    at constants.ts:46:95
    at index.ts:20:52

I've tried on development branch too but still have this issue. it's not about this PR I would say. @pavlovcik

barebind · 2024-02-27T09:29:51Z

Got it, it's /static/scripts/rewards/constants.ts file declaring wrong variable.

-declare const extraRpcs: Record<string, string[]>; // @DEV: passed in at build time check build/esbuild-build.ts
+declare const allNetworkUrls: Record<string, string[]>; // @DEV: passed in at build time check build/esbuild-build.ts
 
 export enum NetworkIds {
   Mainnet = 1,
   Goerli = 5,
   Gnosis = 100,
 }
-console.trace({ extraRpcs });
+console.trace({ allNetworkUrls });

Should I fix it on this PR?

0x4007 · 2024-02-27T12:26:09Z

It should be fixed on most of the other branches why don't you investigate based on the other recently closed pulls and see what commit they are based on.

It seems unusual to me that the problem persists if you rebased (or even merged) from a later commit when it was fixed.

barebind · 2024-02-28T05:37:20Z

added the fix PR #181 for the issue. @pavlovcik

rndquu

Getting an error "updating transaction hash is not allowed". I'm using pavlovcik's supabase public anon key. What am I doing wrong?

static/scripts/rewards/web3/erc20-permit.ts

barebind · 2024-02-28T07:25:43Z

Getting an error "updating transaction hash is not allowed". I'm using pavlovcik's supabase public anon key. What am I doing wrong?

that error is coming from db function. the transaction column is not null so it doesn't allow to update it as we discussed before. but the thing you are supposed to see a toaster saying that the permit is already claimed. let me check it. can you post the permit url?

rndquu · 2024-02-28T09:47:55Z

Getting an error "updating transaction hash is not allowed". I'm using pavlovcik's supabase public anon key. What am I doing wrong?

that error is coming from db function. the transaction column is not null so it doesn't allow to update it as we discussed before. but the thing you are supposed to see a toaster saying that the permit is already claimed. let me check it. can you post the permit url?

Why can we update only null fields? Is there a way to rewrite non null fields via supabase? I just don't get the rationale behind this logic on the supabase side.

rndquu

Works fine

barebind · 2024-02-28T10:58:13Z

Why can we update only null fields? Is there a way to rewrite non null fields via supabase? I just don't get the rationale behind this logic on the supabase side.

Maybe I misunderstood the context but I'm referring these conversations/quotes below.

In case that's too complex to implement: maybe it's simpler to just allow updating if it's null. #173 (comment)

Let it work this way, we'll fix it later when DB schema is stable (ubiquity/ubiquibot#919) #173 (comment)

0x4007 · 2024-02-28T13:25:06Z

I just don't get the rationale behind this logic on the supabase side.

The rationale is that it seemed simple to implement and good for 99% of cases. Given that column level security is an advanced feature according to their docs:

"This is an advanced feature. We do not recommend using column-level privileges for most users.
Instead, we recommend using RLS policies in combination with a dedicated table for handling user
roles."

I figured it makes the most sense to do it the simple way, and then possibly get around to implementing column level security as a lower priority task in the future.

rndquu · 2024-02-28T14:33:46Z

Why can we update only null fields? Is there a way to rewrite non null fields via supabase? I just don't get the rationale behind this logic on the supabase side.

Maybe I misunderstood the context but I'm referring these conversations/quotes below.

In case that's too complex to implement: maybe it's simpler to just allow updating if it's null. #173 (comment)

Let it work this way, we'll fix it later when DB schema is stable (ubiquity/ubiquibot#919) #173 (comment)

I meant if there is way to rewrite non null fields in supabase, because it seems strange that you can write (using supabase's public anon key) to null fields but can't override non empty

barebind · 2024-02-29T04:33:04Z

I agree with the idea that this task focuses solving more like a

show proof that the permit was claimed because I grow tired of addressing this exact same complaint.

sort of problems. it is more about the UX rather than security. I think security concern should be another issue to discuss.

on the other hand though, I totally agree with @rndquu's comment in long term. simply checking not null doesn't mean anything from security standpoint.

0x4007 · 2024-02-29T05:42:05Z

Where is the notification?

https://654635bd.pay-ubq-fi-bxp.pages.dev/?claim=W3sidHlwZSI6ImVyYzIwLXBlcm1pdCIsInBlcm1pdCI6eyJwZXJtaXR0ZWQiOnsidG9rZW4iOiIweGU5MUQxNTNFMGI0MTUxOEEyQ2U4RGQzRDc5NDRGYTg2MzQ2M2E5N2QiLCJhbW91bnQiOiIyNDgwMDAwMDAwMDAwMDAwMDAwMCJ9LCJub25jZSI6IjExNDkzMTA4ODMwNzAxNzM0MDUzMTk4NzM1Nzg0NTE1MTg4Mjk4ODM3MjE4MDE3MjAyNjgyNDQ4ODIyMzY1MDQxMDMwMTM5ODY0MzQ1MSIsImRlYWRsaW5lIjoiMTE1NzkyMDg5MjM3MzE2MTk1NDIzNTcwOTg1MDA4Njg3OTA3ODUzMjY5OTg0NjY1NjQwNTY0MDM5NDU3NTg0MDA3OTEzMTI5NjM5OTM1In0sInRyYW5zZmVyRGV0YWlscyI6eyJ0byI6IjB4NDAwN0NFMjA4M2M3RjNFMTgwOTdhZUIzQTM5YmI4ZUMxNDlhMzQxZCIsInJlcXVlc3RlZEFtb3VudCI6IjI0ODAwMDAwMDAwMDAwMDAwMDAwIn0sIm93bmVyIjoiMHg0NENhMTVEYjEwMWZEMWMxOTQ0NjdEYjZBRjBjNjdDNkJiRjRBQjUxIiwic2lnbmF0dXJlIjoiMHhjMTZjYzIyODNjZjFiZTBiNGFhMTAzZTU4OTZiZmNiNDcyY2FiYWNlNTIyOWFlNmY1ZjRhYWJlNTRkN2FlZTk4MzE4Yzc5NmFkNGRkYjIwNjgxYzQ3OTI1YTViOGM0YTZkZmM2YjVkMTQ1M2E2OTM2YjQ2ZjQxMzEwNzk0Yjk0NzFjIiwibmV0d29ya0lkIjoxMDB9XQ%3D%3D

barebind · 2024-02-29T06:10:41Z

Where is the notification?

pay.ubq.fi/static/scripts/rewards/web3/erc20-permit.ts

Line 93 in 959bbf7

    
           toaster.create("error", `Your reward for this task has already been claimed or invalidated.`);

0x4007 · 2024-02-29T06:21:39Z

Where is the notification?

pay.ubq.fi/static/scripts/rewards/web3/erc20-permit.ts

Line 93 in 959bbf7

toaster.create("error", `Your reward for this task has already been claimed or invalidated.`);

This task is to show the transaction hash of the prior claim. Besides, what you're showing me already has been implemented and is live on pay.ubq.fi

0x4007 · 2024-02-29T06:23:23Z

static/scripts/rewards/web3/erc20-permit.ts

@@ -74,6 +81,13 @@ export function claimErc20PermitHandler(permit: Erc20Permit, provider: JsonRpcPr
 }

 export async function checkPermitClaimable(permit: Erc20Permit, signer: ethers.providers.JsonRpcSigner | null, provider: JsonRpcProvider) {
+  const permitHash = await doesPermitHaveTxHash(permit);
+  if (permitHash !== null) {
+    const explorerLink = `<a href="${getExplorerLinkForTx(permit.networkId, permitHash)}" target=_blank >${shortenTxHash(permitHash)}</a>`;


Oh I understand, its only if it exists in the database. Perhaps we can do an end-to-end test somehow? How did you test locally? We can just do the same.

1- generate claim through script
2- write it to db manually (as null transaction column)
3- and test the rest :)

I assume that (I have to somehow) permits are written to db correctly.

I suppose for another task we should replace the claim button with the transaction hash if it has already been claimed or validated.

We can substitute the claim button with one that has this icon, and upon mouse over it can say "View Claim"

<svg xmlns="http://www.w3.org/2000/svg" height="24" viewBox="0 -960 960 960" width="24"><path d="M212.309-140.001q-30.308 0-51.308-21t-21-51.308v-535.382q0-30.308 21-51.308t51.308-21h252.305V-760H212.309q-4.616 0-8.463 3.846-3.846 3.847-3.846 8.463v535.382q0 4.616 3.846 8.463 3.847 3.846 8.463 3.846h535.382q4.616 0 8.463-3.846 3.846-3.847 3.846-8.463v-252.305h59.999v252.305q0 30.308-21 51.308t-51.308 21H212.309Zm176.46-206.615-42.153-42.153L717.847-760H560v-59.999h259.999V-560H760v-157.847L388.769-346.616Z"/></svg>

barebind · 2024-02-29T06:30:52Z

Where is the notification?

pay.ubq.fi/static/scripts/rewards/web3/erc20-permit.ts

Line 93 in 959bbf7

toaster.create("error", `Your reward for this task has already been claimed or invalidated.`);

This task is to show the transaction hash of the prior claim. Besides, what you're showing me already has been implemented and is live on pay.ubq.fi

There are two different checks,

first one is db check for not-null-transaction-column if there is a hash then show the explorer link

pay.ubq.fi/static/scripts/rewards/web3/erc20-permit.ts

Line 87 in 959bbf7

    
           toaster.create("error", `This reward has already been claimed. Transaction hash: ${explorerLink}`);

and the second one that you are seeing as a toaster message is claimed nonce check.

the order is as it is, first check db and then nonce.

0x4007 · 2024-02-29T06:34:18Z

I'm testing now. Writing some notes. Some unrelated issue:

TypeError: Cannot set properties of null (setting 'innerHTML')
    at renderToFields (insert-table-data.ts:93:10)
    at insertErc20PermitTableData (insert-table-data.ts:15:3)
    at renderTransaction (render-transaction.ts:132:36)
Promise.catch (async)		
handler	@	erc20-permit.ts:72

  toFull.innerHTML = `<div>${receiverAddress}</div>`;

0x4007 · 2024-02-29T06:34:46Z

Doesn't look like it works because I just tested on this.

rndquu · 2024-02-29T07:30:42Z

I've tested this PR locally, not on CF deployments + the tx hash must exist in the DB in order for the notification with tx hash to be displayed

0x4007 · 2024-02-29T08:04:21Z

I've tested this PR locally, not on CF deployments + the tx hash must exist in the DB in order for the notification with tx hash to be displayed

I think this feature should also include writing the data to the database. Even if my permit amount was of 0 it should still be recorded in the database.

barebind · 2024-02-29T08:28:48Z

I think this feature should also include writing the data to the database. Even if my permit amount was of 0 it should still be recorded in the database.

it actually writes the transaction hash to the database after the transaction is sent and confirmed.

I think your test case is an already claimed permit which has a null transaction column in the database. which shouldn't happen normally

0x4007 · 2024-02-29T08:43:43Z

No I generated a new permit and claimed it. You can check the chain for a timestamp.

rndquu · 2024-02-29T09:33:03Z

I've tested this PR locally, not on CF deployments + the tx hash must exist in the DB in order for the notification with tx hash to be displayed

I think this feature should also include writing the data to the database. Even if my permit amount was of 0 it should still be recorded in the database.

This PR does write tx hash to the permits.transaction field (if it is null)

barebind · 2024-02-29T09:38:22Z

No I generated a new permit and claimed it. You can check the chain for a timestamp.

do you generate new permit manually? if the permit is on db with null hash(not claimed) it updates the permits.transaction with the sent transaction's hash.

it doesn't write the permit data to the database.

0x4007 · 2024-02-29T12:06:50Z

I have the real private key in my .env so I generate real permits when I run yarn start.

When is it written to the database? It's a bit difficult for me to see in the code when I'm mobile.

barebind · 2024-02-29T12:52:32Z

this pr is only updating already existing permit with the transaction hash. I was assuming permit is inserted to database when/where it's generated.

barebind · 2024-02-29T12:58:39Z

I have the real private key in my .env so I generate real permits when I run yarn start.

normally the permits are generated(and inserted to db) by ubiquibot so when I'm testing I inserted the generated permit(yarn start) to db manually.

rndquu · 2024-02-29T16:03:23Z

I have the real private key in my .env so I generate real permits when I run yarn start.

normally the permits are generated(and inserted to db) by ubiquibot so when I'm testing I inserted the generated permit(yarn start) to db manually.

Yes, it's expected to work this way:

The bot generates a permit in the DB
Contributor claims the permit
Claim tx is written to a DB

So in order to see a tx url in the permits.transaction field the permit must exist in the DB

0x4007 · 2024-03-05T07:11:05Z

@barebind any updates?

barebind · 2024-03-05T07:36:13Z

@barebind any updates?

I've started working on #182 today which makes this one useless I suppose.
Anything to do with this one?

0x4007 · 2024-03-05T08:29:54Z

@barebind any updates?

I've started working on #182 today which makes this one useless I suppose. Anything to do with this one?

If you think this is obsolete then you can close this pull.

barebind · 2024-03-05T08:34:31Z

If you think this is obsolete then you can close this pull.

seems like you changed this one to draft 2 days ago, any specific reason? anything that you were waiting from me?

in any case I think that we definitely cannot merge both branches as basically they are doing the same thing in different ways.

if you don't have any feedback I think it's better to close this one.

0x4007 · 2024-03-05T08:39:13Z

if you don't have any feedback I think it's better to close this one.

This didn't work in my end-to-end test.

barebind · 2024-03-06T05:01:32Z

I'm closing this PR as it is basically same feature (and same issues) with different UI/UX with #173

Add db checks and explorer links for already claimed permit

5dd1b6f

barebind added 2 commits February 22, 2024 17:26

refactor: add helpers for tx hash

367a0c8

feat: update permit on db after tx confirmed

2602679

0x4007 marked this pull request as ready for review February 26, 2024 10:30

0x4007 self-requested a review as a code owner February 26, 2024 10:30

rndquu reviewed Feb 26, 2024

View reviewed changes

static/scripts/rewards/web3/erc20-permit.ts Show resolved Hide resolved

rndquu mentioned this pull request Feb 26, 2024

Setup RLS and CLS for supabase ubiquity/ubiquibot#919

Closed

Merge branch 'ubiquity:development' into feature/show-tx-hash-for-cla…

b6aa5b4

…imed-permits

barebind mentioned this pull request Feb 28, 2024

Uncaught TypeError: define_extraRpcs #180

Closed

fix: extraRpcs declaration error

b800e77

rndquu self-requested a review February 28, 2024 06:42

rndquu requested changes Feb 28, 2024

View reviewed changes

static/scripts/rewards/web3/erc20-permit.ts Outdated Show resolved Hide resolved

chore: remove todo

959bbf7

barebind requested a review from rndquu February 28, 2024 07:37

rndquu approved these changes Feb 28, 2024

View reviewed changes

0x4007 reviewed Feb 29, 2024

View reviewed changes

0x4007 mentioned this pull request Feb 29, 2024

View Claim Button #182

Closed

0x4007 marked this pull request as draft March 3, 2024 03:55

barebind mentioned this pull request Mar 5, 2024

Feature: View Claim Button #187

Merged

barebind closed this Mar 6, 2024

Feature: Show transaction hashes for claimed permits #173

Feature: Show transaction hashes for claimed permits #173

Conversation

barebind commented Feb 22, 2024

ubiquity-os-deployer bot commented Feb 22, 2024 • edited Loading

0x4007 commented Feb 22, 2024 • edited Loading

0x4007 commented Feb 26, 2024 • edited Loading

barebind commented Feb 27, 2024

rndquu commented Feb 27, 2024

barebind commented Feb 27, 2024

barebind commented Feb 27, 2024

barebind commented Feb 27, 2024

0x4007 commented Feb 27, 2024

barebind commented Feb 28, 2024

rndquu left a comment

Choose a reason for hiding this comment

barebind commented Feb 28, 2024

rndquu commented Feb 28, 2024

rndquu left a comment

Choose a reason for hiding this comment

barebind commented Feb 28, 2024

0x4007 commented Feb 28, 2024 • edited Loading

rndquu commented Feb 28, 2024

barebind commented Feb 29, 2024

0x4007 commented Feb 29, 2024

barebind commented Feb 29, 2024

0x4007 commented Feb 29, 2024 • edited Loading

0x4007 Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

barebind Feb 29, 2024

Choose a reason for hiding this comment

0x4007 Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

barebind commented Feb 29, 2024

0x4007 commented Feb 29, 2024

0x4007 commented Feb 29, 2024

rndquu commented Feb 29, 2024

0x4007 commented Feb 29, 2024

barebind commented Feb 29, 2024

0x4007 commented Feb 29, 2024

rndquu commented Feb 29, 2024

barebind commented Feb 29, 2024

0x4007 commented Feb 29, 2024

barebind commented Feb 29, 2024

barebind commented Feb 29, 2024 • edited Loading

rndquu commented Feb 29, 2024

0x4007 commented Mar 5, 2024

barebind commented Mar 5, 2024 • edited Loading

0x4007 commented Mar 5, 2024

barebind commented Mar 5, 2024 • edited Loading

0x4007 commented Mar 5, 2024

barebind commented Mar 6, 2024

ubiquity-os-deployer bot commented Feb 22, 2024 •

edited

Loading

0x4007 commented Feb 22, 2024 •

edited

Loading

0x4007 commented Feb 26, 2024 •

edited

Loading

0x4007 commented Feb 28, 2024 •

edited

Loading

0x4007 commented Feb 29, 2024 •

edited

Loading

0x4007 Feb 29, 2024 •

edited

Loading

0x4007 Feb 29, 2024 •

edited

Loading

barebind commented Feb 29, 2024 •

edited

Loading

barebind commented Mar 5, 2024 •

edited

Loading

barebind commented Mar 5, 2024 •

edited

Loading