Skip to content
This repository has been archived by the owner on Mar 1, 2024. It is now read-only.

Keep chainId at just a single place #121

Closed
atvanguard opened this issue Sep 28, 2019 · 5 comments · Fixed by #135
Closed

Keep chainId at just a single place #121

atvanguard opened this issue Sep 28, 2019 · 5 comments · Fixed by #135

Comments

@atvanguard
Copy link
Contributor

Requires minor refactoring.

@0xAshish
Copy link
Contributor

@atvanguard actually it was at a single place in the beginning 😆

@atvanguard
Copy link
Contributor Author

atvanguard commented Sep 30, 2019

Well, do you think the reason could be that "in the beginning",

  1. childERC20 / childerc721 tokens did not know how to execute transferWithSig orders for marketplace. That was added here.

  2. We did not have marketplace predicate.

  3. We did not have tests for marketplace

  4. We did not have tests for marketplace predicate.

There was no way you would end up having that duplicated across several files 🤣

@cumpsty
Copy link

cumpsty commented Nov 6, 2019

I'm interested in helping out. Is this a good issue to get started with? Anything I should know?

@atvanguard
Copy link
Contributor Author

atvanguard commented Nov 6, 2019

Hey @cumpsty Thanks for the interest! Glad to hear you'd like to work on it.

Anything I should know?

Nothing specific, just check all the places chainId is being used in contracts and let's keep that at in a single place that others can inherit from. Same for tests. Need this done a little urgently :)

@atvanguard
Copy link
Contributor Author

Hey Cumpsty,
Sorry we needed this done soon, so I rolled out a fix. Happy to help on any other feature you'd like to work on.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants