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

Respect ERC1271 #286

Merged
merged 12 commits into from
Jun 20, 2023
Merged

Respect ERC1271 #286

merged 12 commits into from
Jun 20, 2023

Conversation

sosoxuc
Copy link
Contributor

@sosoxuc sosoxuc commented Jun 14, 2023

Motivation

Allow ERC1271 orders

Solution

Offerer can be different from signer (e.g. contract wallet address)

@ryanio
Copy link
Collaborator

ryanio commented Jun 14, 2023

nice thanks, do you mind also adding a 1271 test to ensure it works and doesn't break in the future?

i think we would also need to skip the signOrder step when 1271?

@sosoxuc
Copy link
Contributor Author

sosoxuc commented Jun 17, 2023

i think we would also need to skip the signOrder step when 1271?

Do we?
An EOA can sign the order and then it is checked inside 1271 contract

    address public signer;

    function isValidSignature(bytes32 hash_, bytes calldata signature_) external override view returns (bytes4) {
        address signer_ = hash_.recover(signature_);
        if (signer_ == signer) {
            return 0x1626ba7e;
        }
        return 0x00000000;
    }

@sosoxuc
Copy link
Contributor Author

sosoxuc commented Jun 17, 2023

nice thanks, do you mind also adding a 1271 test to ensure it works and doesn't break in the future?

Sure, will do

@sosoxuc
Copy link
Contributor Author

sosoxuc commented Jun 17, 2023

nice thanks, do you mind also adding a 1271 test to ensure it works and doesn't break in the future?

Added tests but I have side effect — https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L70 will reject compacted sig

I can revert that change in the code if you do not think that makes sense

@sosoxuc
Copy link
Contributor Author

sosoxuc commented Jun 17, 2023

@ryanio

src/seaport.ts Outdated Show resolved Hide resolved
src/seaport.ts Outdated Show resolved Hide resolved
test/erc1271-order.spec.ts Outdated Show resolved Hide resolved
@ryanio
Copy link
Collaborator

ryanio commented Jun 20, 2023

a couple comments to resolve but otherwise looks great, thanks for doing!

@sosoxuc
Copy link
Contributor Author

sosoxuc commented Jun 20, 2023

@ryanio applied all changes

@ryanio
Copy link
Collaborator

ryanio commented Jun 20, 2023

looks good, could you run npm run prettier:fix please

@sosoxuc
Copy link
Contributor Author

sosoxuc commented Jun 20, 2023

@ryanio done

@ryanio ryanio merged commit 5aac955 into ProjectOpenSea:main Jun 20, 2023
nero-tang added a commit to nero-tang/seaport-js that referenced this pull request Jun 22, 2023
* main: (79 commits)
  Update package.json - bump to v2.0.4 (ProjectOpenSea#291)
  Update Node.js to v16.20.1 (ProjectOpenSea#290)
  Respect ERC1271 (ProjectOpenSea#286)
  Update dependency dotenv to v16.3.1 (ProjectOpenSea#273)
  Update dependency sinon to v15.2.0 (ProjectOpenSea#285)
  Update dependency c8 to v8 (ProjectOpenSea#287)
  Bump @openzeppelin/contracts from 4.9.0 to 4.9.2 (ProjectOpenSea#289)
  Update dependency @types/node to v16.18.36 (ProjectOpenSea#284)
  Update dependency hardhat to v2.15.0 (ProjectOpenSea#282)
  Update dependency @0xsequence/multicall to v0.43.34 (ProjectOpenSea#278)
  Update typescript-eslint monorepo to v5.60.0 (ProjectOpenSea#277)
  Update dependency eslint to v8.43.0 (ProjectOpenSea#271)
  Update dependency typescript to v5.1.3 (ProjectOpenSea#267)
  Update dependency concurrently to v8.2.0 (ProjectOpenSea#266)
  fix postinstall script, bump version (ProjectOpenSea#280)
  Update npm-publish.yml (ProjectOpenSea#276)
  Fix husky postinstall script (ProjectOpenSea#275)
  add readme banner (ProjectOpenSea#270)
  Update package.json version (ProjectOpenSea#269)
  Fix npm-publish.yml (ProjectOpenSea#268)
  ...
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.

2 participants