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

Add ERC-20 Support to NFT Orderbook Smart Contract #469

Open
7 tasks
kwiss opened this issue Sep 25, 2024 · 18 comments
Open
7 tasks

Add ERC-20 Support to NFT Orderbook Smart Contract #469

kwiss opened this issue Sep 25, 2024 · 18 comments
Assignees
Labels
difficulty: medium To resolve the issue, it is necessary to gain an understanding of some aspects of the codebase good first issue Good for newcomers lang:cairo Work on the Starknet part of the application; you need to know Cairo lang. ODHack8 Only dust 8 priority: medium

Comments

@kwiss
Copy link
Contributor

kwiss commented Sep 25, 2024

Description

We need to extend our NFT orderbook smart contract to support ERC-20 tokens in addition to the currently supported token standards. This addition will allow for fungible token trading within our orderbook system.

Current Status

  • The smart contract currently supports ERC-721 tokens and is being updated to support ERC-1155.
  • We need to adapt the structure to accommodate ERC-20 tokens, which have different characteristics from NFTs.

Tasks

  1. Deploy an ERC-20 token contract on a devnet for testing purposes.
  2. Add support for the ERC-20 standard to the orderbook smart contract:
    • Implement dedicated functions for ERC-20 interactions.
    • Modify existing logic to handle token quantities.
    • Implement logic to handle non-existing token IDs (as ERC-20 tokens don't have individual IDs).

Implementation Details

  • The primary changes for ERC-20 support will be:
    1. Handling token quantities (similar to ERC-1155 support).
    2. Removing the concept of token IDs for ERC-20 transactions.
  • Existing functions and data structures should be reviewed and updated to accommodate these changes.
  • New functions specific to ERC-20 operations will need to be added.

Acceptance Criteria

  • ERC-20 token contract successfully deployed on devnet
  • Orderbook smart contract updated to support ERC-20 tokens
  • All existing functionality for other token standards remains intact
  • New ERC-20 specific functions are implemented and tested
  • Quantity handling is correctly implemented for ERC-20 tokens
  • Logic for handling non-existing token IDs is implemented and tested
  • All tests pass, including new tests for ERC-20 functionality

Additional Notes

  • Ensure backwards compatibility with existing supported token standards
  • Update documentation to reflect new ERC-20 support
  • Consider gas optimization when implementing quantity handling
  • Implement appropriate checks to distinguish between different token standards in the contract
@kwiss kwiss added lang:cairo Work on the Starknet part of the application; you need to know Cairo lang. ODHack8 Only dust 8 difficulty: medium To resolve the issue, it is necessary to gain an understanding of some aspects of the codebase good first issue Good for newcomers priority: medium labels Sep 25, 2024
@Iwueseiter
Copy link

Hi @kwiss can I work on this?

@ShantelPeters
Copy link
Contributor

Hi @kwiss please can i be assigned to this issue?

@Jemiiah
Copy link

Jemiiah commented Sep 25, 2024

@kwiss i'd be up for this

@manlikeHB
Copy link

manlikeHB commented Sep 26, 2024

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

Hi, I am Cairo developer with lots of experience contributing to Cairo projects, my OD profile is a witness to this. I've had the experience of extending smart contracts to support either erc20 or erc721, so I believe this is within my comfort zone.

How I plan on tackling this issue

  • I will deploy an erc20 token for testing purposes
  • I will add the support for erc20 token by embedding an erc20 component (either from OZ or a custom one) to the orderbook smart contract and ensure it's interface is compatible with the standard erc20 token
  • I will implement dedicated functions for ERC-20 interactions.
  • Modify existing logic to handle token quantities.
  • Implement logic to handle non-existing token IDs since ERC-20 tokens don't have individual IDs.
  • I will write test covering all edge cases and ensure the new functionality behaves as expected.

@JoE11-y
Copy link

JoE11-y commented Sep 26, 2024

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

With over four years in blockchain and backend development, I’ve worked across different ecosystems, handling everything from smart contract design to on-chain interactions and protocol integration. I focus on building secure, scalable, and reliable blockchain applications, managing both on-chain and off-chain infrastructure.

How I plan on tackling this issue

Would begin by studying the current design of the NFT orderbook, would then proceed to outlining different routes possible to complete the task.

@aji70
Copy link

aji70 commented Sep 26, 2024

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

i'm a solidity and cairo smart contract developer with over 2 years experience and belive i have the skill set for the task

@ryzen-xp
Copy link

ryzen-xp commented Sep 26, 2024

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I'm a Solidity adn cairo developer specializing in NFT marketplaces and decentralized apps, with experience in multi-token support and integrating blockchain protocols. My work on projects like Worldcoin-Bridge-Linea equips me to handle tasks like adding ERC-20 support efficiently.

How I plan on tackling this issue

Deploy an ERC-20 token on a devnet for testing.Review the current code to find areas needing changes to support ERC-20
Add ERC-20 specific logic: implement functions for ERC-20 transfers, handle token quantities, and remove the need for token IDs.
Modify existing logic to manage ERC-20 alongside ERC-721 and ERC-1155.
Test the integration by executing trades with the ERC-20 token.

@MullerTheScientist
Copy link

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I am a full-stack developer with experience in QA testing and languages like Python, Cairo, Solidity, React, and JavaScript.

How I plan on tackling this issue

i will Review Existing Contract
Study the current NFT Orderbook Smart Contract implementation.
Identify areas requiring modifications for ERC-20 support. Define ERC-20 Token Integration
Determine how ERC-20 tokens will interact with the NFT orderbook:
Token types (e.g., payment, discount)
Token usage (e.g., buying, selling)
Implement ERC-20 Token Standards
Integrate ERC-20 token standards (ERC-20 interface):
totalSupply
balanceOf
transfer
approve
allowance

@ScottyDavies
Copy link

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I have a strong background in blockchain development and smart contract programming

How I plan on tackling this issue

To approach the problem, I would:

Create and deploy an ERC-20 token contract on a development network for testing.

Analyze current functions to understand how ERC-721 and ERC-1155 interactions are handled.

Add dedicated functions for ERC-20 interactions, focusing on quantity handling without token IDs.

Update relevant data structures to accommodate token quantities and ensure compatibility with existing standards.

Create checks for non-existing token IDs, as ERC-20 tokens don’t have them.

Ensure all changes maintain backward compatibility with existing token standards.

Develop and run tests for all new and modified functions to confirm correct functionality and gas optimization.

Update the documentation to include details on the new ERC-20 support

@ShantelPeters
Copy link
Contributor

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

i am a blockchain developer with vast knowlegde on javascript, typescript, cario etc, i will be happy if i get assigned to work on this issue

How I plan on tackling this issue

To approach this issue i will do the following:

  1. Deploy ERC-20 Token: Set up an ERC-20 token contract on a devnet for testing.

  2. Update Orderbook Contract:

    • Add functions for ERC-20 interactions.
    • Modify existing logic to handle token quantities and remove token IDs for ERC-20.

3 Testing: Ensure all existing functionalities remain intact, implement new tests for ERC-20 operations, and verify that quantity handling and non-existing token ID logic work correctly.

  1. Documentation: Update the documentation to include the new ERC-20 support and ensure backward compatibility.

  2. Optimization: Consider gas efficiency during implementation.

@kwiss
Copy link
Contributor Author

kwiss commented Sep 26, 2024

hey @JoE11-y you are assigned for now on that, if i don't have news next week you'll be removed

@JoE11-y
Copy link

JoE11-y commented Sep 29, 2024

Alright @kwiss

@JoE11-y
Copy link

JoE11-y commented Sep 30, 2024

Gm @kwiss,

Thanks for the task! I just wanted to clarify a few things to ensure I fully understand the requirements around "allowing fungible token trading within our order book system."

Are we looking to create new order routes, such as "ERC20 -> ERC20," or should I focus on updating the existing routes—"ERC20 -> ERC721" and "ERC721 -> ERC20"—to support additional ERC20 tokens? Cause I noticed that within the code comments of OrderV1, "ERC20" seems to refer to ETH Starkgate.

Lastly, I tried to find references to ERC1155 implementations, but the only mentions I came across were in the Sana and Pontos manager's contract , and mainly related to checking token balances to determine if a token is ERC1155.

I really appreciate any guidance you can provide here, as I’m eager to fully grasp the task and get moving in the right direction! Really excited to get moving.

Thanks in advance.

@kwiss
Copy link
Contributor Author

kwiss commented Oct 1, 2024

just as a reminder you need to use this branch as a base #440 @JoE11-y

@JoE11-y
Copy link

JoE11-y commented Oct 2, 2024

Got it @kwiss

@JoE11-y
Copy link

JoE11-y commented Oct 4, 2024

@kwiss @ptisserand

I think I may have jumped the gun. I started working on system matching the ERC-20 orders, but does this issue include actually matching them, or is it just about storing and fulfilling the orders like the other order types.

Is this the direction we're going for?

@JoE11-y
Copy link

JoE11-y commented Oct 4, 2024

Okay I've reformatted the code. haha

@Snehagupta1907
Copy link

is this issue fixed?or can i take up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium To resolve the issue, it is necessary to gain an understanding of some aspects of the codebase good first issue Good for newcomers lang:cairo Work on the Starknet part of the application; you need to know Cairo lang. ODHack8 Only dust 8 priority: medium
Projects
None yet
Development

No branches or pull requests