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

PRC-4 draft #5

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

PRC-4 draft #5

wants to merge 21 commits into from

Conversation

matejos
Copy link
Contributor

@matejos matejos commented Mar 13, 2024

No description provided.

@matejos matejos requested a review from SebastienGllmt March 13, 2024 15:42
PRCS/prc-4.md Outdated Show resolved Hide resolved
PRCS/prc-4.md Outdated Show resolved Hide resolved
PRCS/prc-4.md Outdated Show resolved Hide resolved
PRCS/prc-4.md Outdated Show resolved Hide resolved
PRCS/prc-4.md Outdated Show resolved Hide resolved
PRCS/prc-4.md Outdated Show resolved Hide resolved
PRCS/prc-4.md Outdated Show resolved Hide resolved
PRCS/prc-4.md Outdated Show resolved Hide resolved
PRCS/prc-4.md Outdated Show resolved Hide resolved
PRCS/prc-4.md Show resolved Hide resolved
@matejos matejos requested a review from SebastienGllmt March 14, 2024 12:47
PRCS/prc-4.md Outdated
3. The user then makes a smart contract call that fulfills all the orders and sends the right amount to the corresponding accounts in a single transaction (atomic).
4. The game chain monitors the successfully fulfilled orders on the base chain and transfers the assets between accounts.

**Note:**
Copy link
Contributor

Choose a reason for hiding this comment

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

Although an increasing index works, it will make the UI slow to update (recall: we had the same problem in the inverse projection where if the inverse projection is base-initiated it takes a while for the game node to update)

In this case however, we can't easily rely on app-side initiation solely because new UTXO entries are created when an order is partially filled (which has a new index)

To solve it though, I think we can take a variation of what we did for the inverse projection system:

  1. Initiate your sell order on Tarochi (by placing in your gold to trade). This will give you a wallet-specific counter (instead of a global counter like we have here)
  2. The RPC endpoint we query to know is https://${rpcBase}/inverseProjection/${purpose}/${chainIdentifier}/ just like in PRC3, but with purpose set to orderbook
  3. Unlike the NFT case where we queried for the file ${address}/${userTokenId}, we instead query for ${address}/${orderId}/${amount}. The endpoint then returns something like { aboveInitial: true } if the amount provided is above the initial tgold deposited by the user in step 1 (note above initial means when the initial utxo was created in this chain)

Note that this requires essentially 2 IDs that we need to work with:

  1. An initial ID
  2. A sub-ID for how far we are in a chain of partial orders

I'm open to thoughts about how we want to handle the combination of these ids. We have 2 options:

  1. The orderID is a combination of initial_id, sub_id>. This means that the game just needs to track orderIDs and it can look up their balance just from that, and this is more faithful to the UTXO model. However, it means functions like cancelSellOrder or fillSellOrders may fail just because the subID changed even though the function call would have succeeded otherwise
  2. The orderID is constant (subID is just a concept that isn't actually in the Solidity contract directly).

Then, like PRC3, we can create a function in this contract (one with the default RPC, one with the user RPC) that constructs the RPC URL from the Solidity contract.

With this configuration, we should end up where the Solidity contract provides an endpoint that always works right away to know if the order is real (has enough tgold). You simply check

  1. if active === true in the Solidity contract
  2. if aboveInitial === true from the RPC call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With partial fills, why would we have to discard the old order and create a new order? I understand that you're thinking in terms of UTxO model, but is this really necessary? We can just have the order track the amount and have it change with partial fills.
Example: There is sell order for 1000 TGOLD. I submit a buy for 800 TGOLD but somebody else submits a buy of 600 TGOLD with higher gas fee, so by the time my transaction is being processed, 600 was already bought and so the sell order would only have 400 TGOLD left. I would much rather have my buy order fill for the 400 TGOLD (because it was a good price per token) than it to revert and me not getting anything and losing money on failed transaction gas fee.

Copy link
Contributor

Choose a reason for hiding this comment

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

With partial fills, why would we have to discard the old order and create a new order

Yes, as I mentioned in my post above we can just do the option orderID is constant. I initially thought we would have to create new orders every time for the game to react based off of it, but I realized that wasn't necessary since the RPC endpoint I mentioned is enough for the game endpoint to return the right state without having to see the tx on Arbitrum

PRCS/prc-4.md Outdated Show resolved Hide resolved
PRCS/prc-4.md Outdated
event OrderCancelled(address indexed seller, uint256 indexed orderId);

/// @notice Returns the seller's current `orderId` (index that their new sell order will be mapped to).
function getSellerOrderId(address seller) external view returns (uint256);
Copy link
Contributor

@SebastienGllmt SebastienGllmt Mar 19, 2024

Choose a reason for hiding this comment

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

Should we add this kind of function to the prc3 contract as well? Ideally the functions and structs between these 2 standards look similar in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it makes too much sense in PRC3, as we only recommend the userTokenId to be address-specific incremental counter.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the benefit of having this function though? If having this function is actually useful for something, maybe we should make it required for PRC3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see immediate benefits in our use case, but it might become useful in the future or for 3rd parties. Better to have this information available than not.

}

event OrderCreated(address indexed seller, uint256 indexed orderId, uint256 assetAmount, uint256 pricePerAsset);
event OrderFilled(
Copy link
Contributor

Choose a reason for hiding this comment

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

We should mention partial fills in the comments somewhere

PRCS/prc-4.md Outdated Show resolved Hide resolved
PRCS/prc-4.md Outdated
/// An order whose `cancelled` parameter has value `true` MUST NOT be filled.
/// MUST change the `assetAmount` parameter for the specified order according to how much of it was filled.
/// MUST emit `OrderFilled` event for each order accordingly.
function fillOrdersExactEth(uint256 minimumAsset, address payable[] memory sellers, uint256[] memory orderIds)
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want a global id for orders we can use as if just need to pass a single id array here it will increase the max number of ids that can be processed in a single tx

unless you had another idea for how to avoid this size limit

Copy link
Contributor Author

@matejos matejos Mar 19, 2024

Choose a reason for hiding this comment

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

Having two arrays instead of one does not pose any threat to hitting the transaction size limit, as realistic scenarios would never come close to the limits (you wouldn't need to be filling 1000 orders at once). I had previously thought about packing the address (20 bytes = uint160) and orderId as uint96 into one uint256, I disregarded that idea because I wanted to keep things simple and it did not really matter storage-wise. But now that you mentioned it, I'm testing for the gas cost difference between these two approaches with regards to function call, and I'm seeing around 2-10% gas cost difference. Do you think it is worth making the standard more complicated (considering this probably won't be deployed to expensive eth mainnet)?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you look at PRC3, we have 2 different IDs (tokenId and userTokenId). The idea was that userTokenId is the wallet-specific one but we can use the global tokenId wherever we don't want to bother passing a <userTokenId, address> pair

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but that's because PRC3 is IERC721, so we had to have global tokenId to conform with the IERC721 in functions like tokenURI(uint256 tokenId). We don't need that here, it would just be a waste of storage - If we want to have the single id, we can simply construct it by packing seller address and seller-specific order id into one uint256 id.

PRCS/prc-4.md Outdated Show resolved Hide resolved
PRCS/prc-4.md Outdated Show resolved Hide resolved
Co-authored-by: Edward Alvarado <ac.edward@gmail.com>
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.

3 participants