Sync ERC permits #177

Keyrxng · 2024-02-25T17:04:59Z

No just Dollar related contracts.

I asked because before I realised my error I thought the sig impl was wrong but I was signing the digest and not the typedData but everything is working now as best as I can test it below is QA that the permit module can generate permits for both ERCs and the UI can process both types.

I'm not sure how to update the DB schema, in audit.ts the below comment is above the permit interface which implies that the schema isn't public/ready yet and I'm sure you've renovated the schema since the last commit in ubiquity/ubiquibot/supabase

// TODO: should be generated directly from the Supabase db schema

Repos modified: [pay.ubq, permit module (bot)] I never had to touch nft-rewards (except adding the setup cast calls).

https://github.com/Keyrxng/ts-template/tree/permit-module

The permits in the link below can be QA'd if you spin things up locally deploying first with acc[0] then setup calls

testing permit plugin ubq-testing/bot-ai#27 (comment)

quad_claim.mp4

caused some type issues related to the two different permit types and passing them to all UI functions.

….ubq.fi into sync-erc-permits

ubiquity-os-deployer · 2024-02-25T17:06:06Z

8392fb7

b7483f7

40b8c3e

e7b3d93

86c4fc6

0x4007 · 2024-02-25T17:09:28Z

I guess the real changes that you made are https://github.com/ubiquity/pay.ubq.fi/compare/b5d1793..2d96f10?expand=1 ?

Keyrxng · 2024-02-25T17:17:39Z

pretty sure everything that's on the left is mine yeah that's good to see I was hesitant because I tried to work from your most recent branch and saw plenty of commits but I feel like I'm getting the hang of it my bad if I'm like a deadweight with it 🤣

0x4007 · 2024-02-25T17:21:32Z

pretty sure everything that's on the left is mine

Okay thanks for clarifying. So then these changes are correct: https://github.com/ubiquity/pay.ubq.fi/compare/2d96f10...b5d1793?expand=1

It will be easier to leave a review if you open a pull request with only your changes.

https://github.com/ubiquity/pay.ubq.fi/compare/2d96f10...b5d1793?expand=1#diff-f694d39e2bd84fb25549e27661bca5cc731479860e7c27ff06f64675dccea6a2R99 - pretty sure somebody renamed the element. You should fix the selector instead. The context is that when there is a lot of screen space available (media query) .toFull was visible if I recall correctly. .toShort was to display truncated addresses I believe.

https://github.com/ubiquity/pay.ubq.fi/compare/2d96f10...b5d1793?expand=1#diff-3e9d257ef7765facfb23435446ce09e602b28d98c0b6b3ba4015d24587d66930R51 - I don't understand why you are prefixing the error message with 1. and 2.

https://github.com/ubiquity/pay.ubq.fi/compare/2d96f10...b5d1793?expand=1#diff-99d32d201d6413a292a4bfe79dbfb7a171dfd8e47b6c1c236f7ad24f2a076e2fR72 - renaming permit.permit to reward.permit is more clear. You should do that wherever possible.

Can you provide some test query params so that we can easily test on the latest commit?

0x4007

Left review above.

Keyrxng · 2024-02-25T17:50:50Z

I don't understand why you are prefixing the error message with 1. and 2.

It made for easier debugging which step was failing

renaming permit.permit to reward.permit is more clear. You should do that wherever possible.

Only in functions that instances of permit.permit occur or all across the board such that I need to update AppState to app.reward as opposed to app.permit?

Only thing you should need to do in order to test is spin up locally

cd /nft-rewards

forge script script/Deploy001_NftReward.s.sol:Deploy001_NftReward --rpc-url http://127.0.0.1:8545 --broadcast -vvvv

cast rpc anvil_impersonateAccount 0xba12222222228d8ba445958a75a0704d566bf2c8 &
cast send 0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d --unlocked --from 0xba12222222228d8ba445958a75a0704d566bf2c8 "transfer(address,uint256)(bool)" 0x70997970C51812dc3A010C7d01b50e0d17dc79C8  337888400000000000000000 &
cast send 0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d --unlocked --from 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266 "approve(address,uint256)(bool)" 0x000000000022D473030F116dDEE9F6B43aC78BA3  9999999999999991111111119999999999999999 &
cast send 0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d --unlocked --from 0x70997970C51812dc3A010C7d01b50e0d17dc79C8 "approve(address,uint256)(bool)" 0x000000000022D473030F116dDEE9F6B43aC78BA3  999999999999999111119999999999999999

0x4007 · 2024-02-25T17:53:28Z

I don't understand why you are prefixing the error message with 1. and 2.

It made for easier debugging which step was failing

renaming permit.permit to reward.permit is more clear. You should do that wherever possible.

Only in functions that instances of permit.permit occur or all across the board such that I need to update AppState to app.reward as opposed to app.permit?

On my not merged branch I changed every instance. permit.permit is unnecessarily confusing.

0x4007 · 2024-02-25T22:20:17Z

forge script script/Deploy001_NftReward.s.sol:Deploy001_NftReward --rpc-url http://127.0.0.1:8545 --broadcast -vvvv

cast rpc anvil_impersonateAccount 0xba12222222228d8ba445958a75a0704d566bf2c8 &
cast send 0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d --unlocked --from 0xba12222222228d8ba445958a75a0704d566bf2c8 "transfer(address,uint256)(bool)" 0x70997970C51812dc3A010C7d01b50e0d17dc79C8  337888400000000000000000 &
cast send 0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d --unlocked --from 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266 "approve(address,uint256)(bool)" 0x000000000022D473030F116dDEE9F6B43aC78BA3  9999999999999991111111119999999999999999 &
cast send 0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d --unlocked --from 0x70997970C51812dc3A010C7d01b50e0d17dc79C8 "approve(address,uint256)(bool)" 0x000000000022D473030F116dDEE9F6B43aC78BA3  999999999999999111119999999999999999

No idea how to do this. Cloned and installed the dependencies on that repo and got

error sending request for url (http://127.0.0.1:8545/): error trying to connect: tcp connect error: Connection refused (os error 61)

Keyrxng · 2024-02-25T22:23:16Z

forge script script/Deploy001_NftReward.s.sol:Deploy001_NftReward --rpc-url http://127.0.0.1:8545 --broadcast -vvvv

cast rpc anvil_impersonateAccount 0xba12222222228d8ba445958a75a0704d566bf2c8 &
cast send 0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d --unlocked --from 0xba12222222228d8ba445958a75a0704d566bf2c8 "transfer(address,uint256)(bool)" 0x70997970C51812dc3A010C7d01b50e0d17dc79C8  337888400000000000000000 &
cast send 0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d --unlocked --from 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266 "approve(address,uint256)(bool)" 0x000000000022D473030F116dDEE9F6B43aC78BA3  9999999999999991111111119999999999999999 &
cast send 0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d --unlocked --from 0x70997970C51812dc3A010C7d01b50e0d17dc79C8 "approve(address,uint256)(bool)" 0x000000000022D473030F116dDEE9F6B43aC78BA3  999999999999999111119999999999999999

No idea how to do this. Cloned and installed the dependencies on that repo and got

error sending request for url (http://127.0.0.1:8545/): error trying to connect: tcp connect error: Connection refused (os error 61)

I comment out the RPC racing stuff usually and just return "http://127.0.0.1:8545" for both the name and url inside the optimal provider, occasionally it doesn't throw an error but easier to comment it out

0x4007 · 2024-02-25T22:24:12Z

Sorry but I am not familiar with anvil etc. @rndquu can probably handle this review.

rndquu · 2024-02-27T14:15:40Z

I don't really understand the original issue. ERC20 permits are about token transfers and amounts while ERC721 permits are about NFT transfers with metadata. They are totally different so it's expected that they don't conform to a single interface.

I will QA this PR.

rndquu · 2024-02-27T14:39:59Z

@Keyrxng I've generated an ERC20 permit for the 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266 address via this script and getting this error: "Please connect your wallet to collect this reward"

It seems that cortex.hiphop is ENS name for the 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266 address which is the 1st address from test mnemonic test test test test test test test test test test test junk

What am I doing wrong?

Keyrxng · 2024-02-27T18:19:22Z

What am I doing wrong?

I'm color blind so it's difficult for me to see in lightmode, but zooming in on my phone I'm sure that the little account connected icon is greyed out meaning that wallet wasn't connected to the ui, clicking the little globe allows you to manually connect (it may not have prompted if mm was already logged into before interacting with the portal)

I've pushed the same localhost rpc fix that I pushed to the page load pr which might help

The idea is that chain_id should be 31337 if testing locally (to avoid hitting real RPCs) and vice versa if testing the rpcs

anvil --chain-id 31337 --fork-url https://rpc.gnosis.gateway.fm

cast rpc anvil_impersonateAccount 0xba12222222228d8ba445958a75a0704d566bf2c8 &
cast send 0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d --unlocked --from 0xba12222222228d8ba445958a75a0704d566bf2c8 "transfer(address,uint256)(bool)" 0x70997970C51812dc3A010C7d01b50e0d17dc79C8  337888400000000000000000 &
cast send 0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d --unlocked --from 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266 "approve(address,uint256)(bool)" 0x000000000022D473030F116dDEE9F6B43aC78BA3  9999999999999991111111119999999999999999 &
cast send 0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d --unlocked --from 0x70997970C51812dc3A010C7d01b50e0d17dc79C8 "approve(address,uint256)(bool)" 0x000000000022D473030F116dDEE9F6B43aC78BA3  999999999999999111119999999999999999

These are my only setup commands beyond yarn start

0x4007 · 2024-02-28T01:11:06Z

I don't really understand the original issue. ERC20 permits are about token transfers and amounts while ERC721 permits are about NFT transfers with metadata. They are totally different so it's expected that they don't conform to a single interface.

I will QA this PR.

Most of the properties can be shared. For example, to, from, amount (1)

The 721 interface must extend the 20 interface. It seemed like only half the property names matched for representing the same data.

This makes dealing with these permits unnecessarily complex.

The NFT specific properties can be inside of a single new "nft" property on the object.

Keyrxng · 2024-02-28T11:53:02Z

0fc24a2 I didn't realise that the eth_requestAccounts call was removed, my wallet would have been connected to my local instance but after checking out the deploy previews I wasn't able to connect either

The deploy comments haven't updated so I'm unable to check if it's working on the deployment(the run succeeded in my repo (other than husky install) but it's requesting my wallet locally (I disconnected and tried it)

rndquu · 2024-02-29T16:51:58Z

I don't really understand the original issue. ERC20 permits are about token transfers and amounts while ERC721 permits are about NFT transfers with metadata. They are totally different so it's expected that they don't conform to a single interface.
I will QA this PR.

Most of the properties can be shared. For example, to, from, amount (1)

The 721 interface must extend the 20 interface. It seemed like only half the property names matched for representing the same data.

This makes dealing with these permits unnecessarily complex.

The NFT specific properties can be inside of a single new "nft" property on the object.

Yes, you're right, most properties can be shared

ERC20 permit:

[
  {
    "type": "erc20-permit",
    "permit": {
      "permitted": {
        "token": "0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d",
        "amount": "100000000000000"
      },
      "nonce": "10619088011567005175430320300177209575218124356573342664785791883534961913741",
      "deadline": "115792089237316195423570985008687907853269984665640564039457584007913129639935"
    },
    "transferDetails": {
      "to": "0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266",
      "requestedAmount": "100000000000000"
    },
    "owner": "0xBA742bdBf4eF1e6be8a917Fd2f84BE6C74F8796B",
    "signature": "0x7d728b4563799a52aec9624009fad67e04a2a2374cb6c19f387d8dbfacb8e2326b035807c01b0973ad009758f015720d1c8f0154ae5a7fd61bfc4596b32c7c471b",
    "networkId": 31337
  }
]

ERC721 permit:

const mintRequest = {
    beneficiary: beneficiaryWallet.address,
    deadline: 9999999999,
    keys: [
        utils.keccak256(utils.toUtf8Bytes("GITHUB_ORGANIZATION_NAME")),
        utils.keccak256(utils.toUtf8Bytes("GITHUB_REPOSITORY_NAME")),
        utils.keccak256(utils.toUtf8Bytes("GITHUB_ISSUE_ID")),
        utils.keccak256(utils.toUtf8Bytes("GITHUB_CONTRIBUTION_TYPE")),
    ],
    nonce: 19246288,
    values: [
        "ubiquity",
        "nft-rewards",
        "1",
        "issue_solver",
    ]
};

My concern is that while both permits have common properties for ease of future debugging it would be better to be explicit and distinguish ERC20 and ERC721 permits (as whilefoo initially implemented) since ERC20 permits don't have domain and types to sign. So any small error will make signature invalid and debugging this ERC20/ERC721 combined permit will take quite a time.

The PR works fine by the way

rndquu · 2024-02-29T16:52:26Z

@Keyrxng There seems to be some code conflicts

0x4007 · 2024-03-01T08:14:37Z

My concern is that while both permits have common properties for ease of future debugging it would be better to be explicit and distinguish ERC20 and ERC721 permits (as whilefoo initially implemented) since ERC20 permits don't have domain and types to sign. So any small error will make signature invalid and debugging this ERC20/ERC721 combined permit will take quite a time.

Hard for me to say with certainty as I'm not that familiar with the smart contract side.

However, based on what I've seen within the UI code, debugging won't be necessary at runtime with my plan. Instead we can have a function that does something like

function checkIfNft(permit: Erc20Permit | Erc721Permit) {
  return !!permit.nft
}

However the proper way would be to type guard it. It's a bit more involved though but it will catch errors in the text editor.

0x4007 · 2024-03-01T08:20:37Z

ChatGPT generic type guard example:

interface Erc20Permit {
  // Assuming there's a unique property for Erc20Permit for demonstration
  tokenAddress: string;
}

interface Erc721Permit {
  // Assuming nft is unique to Erc721Permit
  nft: boolean;
}

// Type guard for Erc721Permit
function isErc721Permit(permit: any): permit is Erc721Permit {
  return (permit as Erc721Permit).nft !== undefined;
}

// Type guard for Erc20Permit
function isErc20Permit(permit: any): permit is Erc20Permit {
  return (permit as Erc20Permit).tokenAddress !== undefined;
}

// Example usage
const permit: Erc20Permit | Erc721Permit = /* some permit object */;

if (isErc721Permit(permit)) {
  console.log('The permit is for an ERC721 token');
  // TypeScript knows permit is Erc721Permit here
} else if (isErc20Permit(permit)) {
  console.log('The permit is for an ERC20 token');
  // TypeScript knows permit is Erc20Permit here
}

Keyrxng · 2024-03-02T12:07:22Z

I've added the type guard, should we wait and add in the rpc package before merging this in?

0x4007 · 2024-03-02T16:52:16Z

Not sure but normally I'd say never wait for merging. You can instead make a new branch and pre merge what needs to be pushed forward on.

0x4007 · 2024-03-03T03:56:57Z

Is this ready to be merged in?

Keyrxng · 2024-03-03T06:08:51Z

Yes it is good to go, I'm sure

molecula451 · 2024-03-03T19:39:35Z

this PR should not have been merged in this repository, this PR make changes to audit, and audit its in its own repo now, not sure why it was merge without taking this in count, what it's the value of this changing in this repo and not on audit

i think some changes are part of this repo, but not all

rndquu · 2024-03-04T08:30:20Z

Breaking pay.ubq.fi into separate repos gave us a mess, I don't know how to merge this PR into 3 different repos since all of them are heavily intertwined and this PR touches all of them

rndquu · 2024-03-04T08:55:50Z

@Keyrxng Sorry for the inconvenience with all of this breaking of pay.ubq.fi into separate repos. Could you spend some more time to make sure this PR ends in https://github.com/ubiquity/pay.ubq.fi, https://github.com/ubiquity/onboard.ubq.fi and https://github.com/ubiquity/audit.ubq.fi? We could create a new issue, just provide a time estimate.

0x4007 and others added 21 commits February 21, 2024 23:18

fix: merge bugs and working on stability

8a09cec

chore: commit hash now in esbuild process

faf7219

refactor: rpc only checks and selects once at session start

80ea9ea

fix: use fastest rpc endpoint for requests

0fc24a2

chore: kebabalize

cef5280

feat: update imports in kebablize

dae1037

feat: mac support

0ff7042

revert: kebabalize only files names

f4c71ec

chore: kebabed

ae73eee

feat: fast claims

c065b8c

fix: post claim render bug

f9c2b87

fix: toaster errors on claims for erc20

2c8f5ca

fix: loader animation on claim button

b9f9c77

refactor: move app into all functions

f307223

caused some type issues related to the two different permit types and passing them to all UI functions.

chore: erc permits first pass

2d96f10

feat: permit second pass working

8693e52

fix: 721 permit from signTyped vs static

6a57f38

fix: exclude node_modules from kebab workflow

a4d0750

fix: pass permit into permit2 and align 721 permit with anvil depl

b5d1793

Merge branch 'ubiquity:development' into sync-erc-permits

ee787e5

Merge branch 'sync-erc-permits' of https://github.com/ubq-testing/pay…

8392fb7

….ubq.fi into sync-erc-permits

Keyrxng requested review from 0x4007 and rndquu as code owners February 25, 2024 17:05

0x4007 requested changes Feb 25, 2024

View reviewed changes

Keyrxng mentioned this pull request Feb 26, 2024

feat: permit module ubiquity-os/permit-generation#1

Merged

chore: no rpc mapping for localhost

40b8c3e

Keyrxng mentioned this pull request Feb 28, 2024

Implement Dynamic RPC pick handler ubiquity/rpc-handler#3

Closed

fix: request account

1d5bb65

rndquu approved these changes Feb 29, 2024

View reviewed changes

Merge branch 'development' into sync-erc-permits

e7b3d93

chore: add permit type guard

86c4fc6

0x4007 merged commit e96d10c into ubiquity:development Mar 3, 2024

This was referenced Mar 3, 2024

Revert "Sync ERC permits" #185

Closed

chore: remove other apps, update readme #186

Merged

rndquu mentioned this pull request Mar 12, 2024

Sync ERC20 Permit and ERC721 Types System Wide ubiquity/.github#98

Closed

Sync ERC permits #177

Sync ERC permits #177

Uh oh!

Conversation

Keyrxng commented Feb 25, 2024 • edited by rndquu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ubiquity-os-deployer bot commented Feb 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0x4007 commented Feb 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Keyrxng commented Feb 25, 2024

Uh oh!

0x4007 commented Feb 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0x4007 left a comment

Choose a reason for hiding this comment

Uh oh!

Keyrxng commented Feb 25, 2024

Uh oh!

0x4007 commented Feb 25, 2024

Uh oh!

0x4007 commented Feb 25, 2024

Uh oh!

Keyrxng commented Feb 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0x4007 commented Feb 25, 2024

Uh oh!

rndquu commented Feb 27, 2024

Uh oh!

rndquu commented Feb 27, 2024

Uh oh!

Keyrxng commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0x4007 commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Keyrxng commented Feb 28, 2024

Uh oh!

rndquu commented Feb 29, 2024

Uh oh!

rndquu commented Feb 29, 2024

Uh oh!

0x4007 commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0x4007 commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Keyrxng commented Mar 2, 2024

Uh oh!

0x4007 commented Mar 2, 2024

Uh oh!

0x4007 commented Mar 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Keyrxng commented Mar 3, 2024

Uh oh!

molecula451 commented Mar 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rndquu commented Mar 4, 2024

Uh oh!

rndquu commented Mar 4, 2024

Uh oh!

Uh oh!

Keyrxng commented Feb 25, 2024 •

edited by rndquu

Loading

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

edited

Loading

0x4007 commented Feb 25, 2024 •

edited

Loading

0x4007 commented Feb 25, 2024 •

edited

Loading

Keyrxng commented Feb 25, 2024 •

edited

Loading

Keyrxng commented Feb 27, 2024 •

edited

Loading

0x4007 commented Feb 28, 2024 •

edited

Loading

0x4007 commented Mar 1, 2024 •

edited

Loading

0x4007 commented Mar 1, 2024 •

edited

Loading

0x4007 commented Mar 3, 2024 •

edited

Loading

molecula451 commented Mar 3, 2024 •

edited

Loading