Skip to content

V1.1.0 rc.3 #521

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.

Sign up for GitHub

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

Jump to bottom
Merged
Parent: Merge develop 1.1.0 into main
merged 12 commits into from
May 26, 2025
Merged

V1.1.0 rc.3 #521

merged 12 commits into from
May 26, 2025

Conversation

@0xlucian
Copy link
Collaborator

No description provided.

@0xlucian 0xlucian requested a review from zajck May 23, 2025 12:09
@0xlucian 0xlucian self-assigned this May 23, 2025
zajck
zajck reviewed May 23, 2025
View reviewed changes
Copy link
Contributor

@zajck zajck left a comment

Choose a reason for hiding this comment

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

Looks good, but tests are failing.

@0xlucian 0xlucian requested a review from zajck May 23, 2025 15:51
@0xlucian
Copy link
Collaborator Author

Looks good, but tests are failing.

Should be fixed now. We had to specify the burn function to be restricted as only burn restricts also FermionFNFT:burn(tokenId)

@zajck
Copy link
Contributor

zajck commented May 26, 2025

Looks good, but tests are failing.

Should be fixed now. We had to specify the burn function to be restricted as only burn restricts also FermionFNFT:burn(tokenId)

@0xlucian FermionFNFT:burn(tokenId) should be restricted, so this change was not in the right direction.

Please change RESTRICTED_METATX_FUNCTIONS back and change the tests to use some other function that fails when forwarded.

@0xlucian
Copy link
Collaborator Author

Looks good, but tests are failing.

Should be fixed now. We had to specify the burn function to be restricted as only burn restricts also FermionFNFT:burn(tokenId)

@0xlucian FermionFNFT:burn(tokenId) should be restricted, so this change was not in the right direction.

Please change RESTRICTED_METATX_FUNCTIONS back and change the tests to use some other function that fails when forwarded.

yes you are right. Those are restricted functions. I have aligned the tests to use public non-restricted functions and to fail. Please re-review if this makes sense

zajck
zajck requested changes May 26, 2025
View reviewed changes
test/protocol/metaTransactionFacet.ts
message.functionSignature,
message.nonce,
[
"0x84ddd3eaf623d5beffc2a66af9525f5cebbe080046f772e1b70df2b7eae63daa",
Copy link
Contributor

Choose a reason for hiding this comment

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

wrap should be restricted too (it's on the list here: https://docs.google.com/document/d/1eyU-DadrJ5Cx7IA-lumDZHlT62ttk8tY42d9S66pd90/edit?tab=t.0 )

transfer that you used in the other test is a better choice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

using transferFrom as further suggested from you

@0xlucian 0xlucian requested a review from zajck May 26, 2025 09:45
zajck
zajck approved these changes May 26, 2025
View reviewed changes
Copy link
Contributor

@zajck zajck left a comment

Choose a reason for hiding this comment

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

LGTM

@0xlucian 0xlucian mentioned this pull request May 26, 2025
Merged
@0xlucian 0xlucian changed the title V1.1.0 rc.3 branch V1.1.0 rc.3 May 26, 2025
@0xlucian 0xlucian merged commit a8a3e5a into develop-1.1.0 May 26, 2025
7 checks passed
@zajck zajck deleted the v1.1.0-rc.3-branch branch June 5, 2025 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zajck zajck zajck approved these changes

Assignees

@0xlucian 0xlucian

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@0xlucian @zajck