Sync ERC20 Permit and ERC721 Types System Wide

0x4007 commented 4 months ago

This is a large project because it requires two tasks:

Designing a robust and sensible schema for all permits in our system.
Updating the code across all of our repositories. This will require updating the database to accomodate the updated schema as well.

This is especially relevant in the:

pay.ubq.fi interface
the UbiquiBot permit generation code
audits app
nft-rewards repository

I'm having type issues due to trying to process ERC20 and ERC721 permits in the UI code (the schemas don't line up at all.)

We should try and sync the properties as much as possible. I'm working on that now on my branch. I hope that it won't cause issues but I'm looking for input on constraints for the property names.

From what I understand if we change the property names then the old permits will be incompatible.

@whilefoo why not just copy the schema as much as possible of ERC20 permits to the ERC721 permits?

_Originally posted by @pavlovcik in https://github.com/ubiquity/pay.ubq.fi/pull/146#discussion_r1498792922_

Keyrxng commented 4 months ago

/start

ubiquibot[bot] commented 4 months ago




Deadline Sat, Feb 24, 4:49 AM UTC

Registered Wallet
0xAe5D1F192013db889b1e2115A370aB133f359765

Tips:

Use /wallet 0x0000...0000 if you want to update your registered payment wallet address.
Be sure to open a draft pull request as soon as possible to communicate updates on your progress.
Be sure to provide timely updates to us when requested, or you will be automatically unassigned from the task.

Keyrxng commented 4 months ago

So as far as I understand it, so long as we maintain the permitTransferFrom type in both nothing should break.

ERC721 recovers against the request object final hash
ERC20 recovers against the PermitTransferFrom object
I've had to hardcode a working sig for now as I've been unable to successfully generate it on the fly for some reason (tried many different ways but cannot generate a valid sig through the script, so input there would be appreciated. (all parts where identical))

The txData (what gets base64 encoded) can be arbitrary as it's not the txData object that's signed against but parts of it.

When building the tally-cli, I noticed a fair few different permit structures although I'm confident that updating the txData object (applying networkId and type etc, for me at least) my broken links were fixed in unspent permits.

Maybe I'm missing the point, what is the fear with backwards compatibility? The actual signed PermitTransferFrom is the source of truth while everything else is metadata for the UI no?

ERC721Permit

https://github.com/ubiquity/.github/assets/106303466/e6902c4f-5bfa-4ece-a6d7-589d280b0918

0x4007 commented 4 months ago

I've had to hardcode a working sig for now as I've been unable to successfully generate it on the fly for some reason (tried many different ways but cannot generate a valid sig through the script, so input there would be appreciated. (all parts where identical))

@Whilefoo @rndquu rfc

Maybe I'm missing the point, what is the fear with backwards compatibility?

Let's consider everything up to now just r&d and not worry about accommodating the data to get into our full version 1.0 production data. It will save us time and energy because there were so many iterations and schemas to accommodate in the past

Keyrxng commented 4 months ago

So long as the schemas have always incorporated the PermitTransferFrom which I believe they have this should be pretty straightforward, in terms of UI's we just need to force previous permits to conform and nothing will break as the actual signed data has always adhered to the permit2-sdk.

Is the issue with backwards compatibility related to the metadata more so than the signature data, I'm assuming it's the sig data?

I just noticed this for generating nft permits this is static too

Keyrxng commented 4 months ago

temporary (maybe not) solution for the permit module at least for simple testing

Designing a robust and sensible schema for all permits in our system.

If I'm understanding right and the compatibility issues are based on the metadata surrounding the signed tx details, then this is done basically

Updating the code across all of our repositories. This will require updating the database to accommodate the updated schema as well.

As for updating the DB the ERC20 schema hasn't changed and the ERC721 structure is a duplicate of within the nftMetadata and request objects appended. I'm unsure what the best way to update the DB schema would be

pay.ubq.fi interface: Working
audits app: Working
nft-rewards repository: Working as required as part of pay UI QA video
the UbiquiBot permit generation code: haven't tried the new permit shape but the module is working for ERC20, will finish up in the next day or so

Once I've verified that the module can create ERC721 permits and then those permits can be claimed via the portal then that's the majority of this task complete isn't it? I'll open PRs on each repo I've touched and link them all back to this?

0x4007 commented 4 months ago

I'm expecting that we need to update the schema on the NFT smart contract and redeploy as well.

Keyrxng commented 4 months ago

Not necessarily, only the request part of the base64 txData is signed on chain for the reward NFT

getMintDigest() returns a hash of the MintRequest struct which is signed and the sig is injected into the final txData which gets base64'd

The UI needs the ERC721 permit to adhere to the signed part of the ERC20 txData in order to unwrap it but including it in the txData when the permit is generated doesn't affect what's being signed onchain

So unless you want to include more of the metadata to what's being signed then it doesn't have to change

In regards to backwards compatibility, with the airdrop-cli it should be easy to bundle all unspent transactions then add the missing metadata the UI needs with new urls or roll them all into one new shape permit

Was the NFT-rewards contract part of the Sherlock audit?

0x4007 commented 4 months ago

Was the NFT-rewards contract part of the Sherlock audit?

No just Dollar related contracts.

Keyrxng commented 3 months ago

I think something has went awry with this one seeing as the associated PR was merged 2 weeks ago

rndquu commented 3 months ago

I think something has went awry with this one seeing as the associated PR was merged 2 weeks ago

Was this PR merged in our other repos mentioned in this comment?

Keyrxng commented 3 months ago

I think something has went awry with this one seeing as the associated PR was merged 2 weeks ago

Was this PR merged in our other repos mentioned in this comment?

I do believe so Paul had me apply the changes soon after merge

There is no usage of any permit types in onboarding as far as I can tell. The /rewards dir still existed but I've just opened a PR to purge that.

rndquu commented 3 months ago

I think something has went awry with this one seeing as the associated PR was merged 2 weeks ago

Was this PR merged in our other repos mentioned in this comment?

I do believe so Paul had me apply the changes soon after merge

chore: remove other apps, update readme pay.ubq.fi#186

ubiquity/audit.ubq.fi@4181d26

There is no usage of any permit types in onboarding as far as I can tell. The /rewards dir still existed but I've just opened a PR to purge that.

Did you get the permit reward for this issue? It was closed (as far as I remember) and reopened later but I don't see any permit URLs

Keyrxng commented 3 months ago

Did you get the permit reward for this issue?

Not as far as I remember no, nothing generated here and I haven't received any manual payments and those other PRs I went with Related to <this issue>

ubiquibot[bot] commented 3 months ago

+ Evaluating results. Please wait...

ubiquibot[bot] commented 3 months ago

[ 33.4 WXDAI ]

@pavlovcik

Contributions Overview

View	Contribution	Count	Reward
Issue	Specification	1	18.6
Issue	Comment	3	14.8

Conversation Incentives

Comment	Formatting	Relevance	Reward
This is a large project because it requires two tasks: 1. Des...	18.6 li: count: 6 score: "6" words: 47	1	18.6
> I've had to hardcode a working sig for now as I've been unable...	10	0.81	10
I'm expecting that we need to update the schema on the NFT smart...	3.8	0.52	3.8
> Was the NFT-rewards contract part of the Sherlock audit? No...	1	0.69	1

[ 400 WXDAI ]

@Keyrxng

Contributions Overview

View	Contribution	Count	Reward
Issue	Task	1	400
Issue	Comment	7	0

Conversation Incentives

Comment	Formatting	Relevance	Reward
So as far as I understand it, so long as we maintain the ``permi...	- a: count: 3 score: "0" words: 4 li: count: 4 score: "0" words: 23 code: count: 4 score: "0" words: 4	0.79	-
So long as the schemas have always incorporated the ``PermitTran...	- a: count: 1 score: "0" words: 1 code: count: 2 score: "0" words: 3	0.67	-
- https://github.com/Keyrxng/ts-template/tree/permit-module - h...	- li: count: 6 score: "0" words: 69 code: count: 2 score: "0" words: 2	0.62	-
Not necessarily, only the ``request`` part of the base64 txData ...	- code: count: 3 score: "0" words: 3	0.53	-
I think something has went awry with this one seeing as the asso...	-	0.64	-
> > I think something has went awry with this one seeing as the ...	- a: count: 2 score: "0" words: 2 li: count: 2 score: "0" words: 18 code: count: 2 score: "0" words: 2	0.64	-
> Did you get the permit reward for this issue? Not as far as...	- code: count: 1 score: "0" words: 6	0.58	-

[ 14 WXDAI ]

@rndquu

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	2	14

Conversation Incentives

Comment	Formatting	Relevance	Reward
> I think something has went awry with this one seeing as the as...	3.2 a: count: 2 score: "2" words: 2	0.64	3.2
> > > I think something has went awry with this one seeing as th...	10.8 a: count: 4 score: "4" words: 17 li: count: 2 score: "2" words: 39 code: count: 2 score: "2" words: 2	0.69	10.8

ubiquity / .github