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

document dynamic primitive (erc-721) #41

Merged
merged 5 commits into from
May 24, 2024

Conversation

ecioppettini
Copy link
Contributor

documentation for PaimaStudios/paima-engine#362

@ecioppettini ecioppettini self-assigned this May 15, 2024
@@ -0,0 +1,53 @@
# Dynamic ERC721
Copy link
Contributor

@SebastienGllmt SebastienGllmt May 17, 2024

Choose a reason for hiding this comment

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

I feel this is a conceptually complex feature with multiple subtle things to take into account and I feel the docs are too short to properly be addressing all of these. Additionally, it's not clear from the docs how you would write a smart contract in a way to trigger dynamic primitive so we should probably provide a concrete example of this and discuss best practices like factory contracts

readonlyDBConn: Pool,
parent: string
): Promise<{ name: string; config: string }[]>;
```

## Performance implications
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably a block of text is not the most readable way to convey this. I would suggest either a a picture or a list of steps. Something like:

During a typical execution, the steps are as follows:
1. Fetch all primitives for a block range concurrently (depending on `funnelBlockGroupSize` or
`presyncStepSize`)
2. Construct the blocks from the fetched data to feed to the state machine

However, dynamic primitive can change the set of primitives the rollup monitors (and therefore change what has to be fetched for a given block range). Therefore, if there are dynamic primitives configured, funnels will instead
1. request all of the events for dynamic primitives and update the set of primitives tracked by the rollup
2. Fetch primitives (both the static ones and the new ones after the dynamic primitive update)
3. Send blocks to the state machine as before

This extra step introduces a performance reduction (whose impact depends how long it takes to fetch data from the node your game is connecting to)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the rollup monitors

why rollup? what are we using that term for? I'm not sure I get it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Your game is a rollup that monitors multiple primitives
https://blog.paimastudios.com/self-sovereign-rollup


There is also a public utility function to get the list of all of the
dynamically generated extensions. In this case the `config` field is formatted
as json. It can be used to extract, the contract address, for example.
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably this should contain a link to configuration.md#Extensions so they know which JSON format we're talking about

I'm assuming you're talking about the extensions config? I'm just guessing this is what you were referring to.
Does this only return a subset of that config file though?

fields:
contractAddress: address
targetConfig:
type: "erc721"
Copy link
Contributor

Choose a reason for hiding this comment

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

We didn't add the ability for some fields to be optionally present, did we?

  • If so, we should document this feature
  • If not, we should create a Github issue so we don't forget this idea and we can address it some point in the future if it comes up

@@ -1,7 +1,13 @@
# Dynamic ERC721
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be better to just call this a "dynamic primitive" in general and just list which standards we support inside the page somewhere (currently only ERC721). Otherwise, we're going to have to copy the entire page content for every primitive we add dynamic support for (and if we support the partial fields in the config, it might end up being all of them)

#### Dynamic configuration

- `contractAddress` has the name of the field in the emitted event that contains
the address of the ERC721.

The example configuration assumes that the event has the following signature:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The example configuration assumes that the event has the following signature:
Solidity has no standardized way to create a factory contract. Instead, it's up to you to create your own factory contract whatever way works best, and to emit events that trigger the dynamic primitive inside it.
The example configuration assumes that the event (such as in your factory contract) has the following signature:

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 also make it very clear whether or not the event emission can contain more parameters than needed, and if the position of the argument matters

triggered by a generic smart contract event. This works by having a smart
contract that has an event that acts as a *trigger* to the engine, and the
engine in response adds a new extension, without needing to change the
configuration file. An use-case for this would be a factory contract, which
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
configuration file. An use-case for this would be a factory contract, which
configuration file.
A use-case for this would be a factory contract, which


The example configuration assumes that the event has the following signature:

```solidity
event CustomEvent(address indexed address);
event CustomEvent(address indexed nftAddress);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
event CustomEvent(address indexed nftAddress);
// note: the name of the argument nftAddress has to match the name specified in dynamicFields
event CustomEvent(address indexed nftAddress);

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want a full example of a Solidity contract for this, we can use

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import {IERC721} from "@openzeppelin/contracts/token/ERC721/IERC721.sol";
import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol";

contract CustomERC721 is ERC721 {
    constructor(string memory name, string memory symbol) ERC721(name, symbol) {}
}

contract FactoryERC721 {
    event ERC721Created(address indexed nftAddress); // emitted when ERC721 token is deployed

    /// @dev deploys an ERC721 token with given parameters
    /// @return address deployed
    function deployERC721(string memory _name, string memory symbol) public returns (address) {
        CustomERC721 t = new CustomERC721(_name, symbol);
        emit ERC721Created(address(t));
        return address(t);
    }
}

@SebastienGllmt SebastienGllmt merged commit 4672e00 into main May 24, 2024
1 check passed
@SebastienGllmt SebastienGllmt deleted the enzo/document-dynamic-erc721 branch May 24, 2024 14:07
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