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

Projected NFT funnel #259

Merged
merged 25 commits into from
Dec 29, 2023
Merged

Conversation

gostkin
Copy link
Contributor

@gostkin gostkin commented Nov 28, 2023

Projected NFT Funnel

Setup

  • Add extensions.yml to folder where the game binary is located:
extensions:
  - name: "Cardano Projected NFT"
    type: cardano-projected-nft
    contract: "703ec99926211daafe291951b778d4da7736a8b0da2aeb92305b511a72"
    startSlot: 43201686
    scheduledPrefix: cpnft
  • Add vars to .env:
CARP_URL=http://localhost:3000
CARDANO_NETWORK=preprod
CARDANO_CONFIRMATION_DEPTH=10

@ecioppettini ecioppettini force-pushed the carp-funnel-stake-delegation-cde branch from 6080ca7 to 879218d Compare November 30, 2023 02:39
@gostkin gostkin changed the title Initial setups Projected NFT funnel Dec 4, 2023
@gostkin gostkin force-pushed the egostkin/paima-projected-nft-funnel branch 4 times, most recently from 820071e to 1b8f719 Compare December 8, 2023 12:49
@gostkin gostkin marked this pull request as ready for review December 8, 2023 12:52
Base automatically changed from carp-funnel-stake-delegation-cde to master December 12, 2023 21:55
@gostkin gostkin force-pushed the egostkin/paima-projected-nft-funnel branch from fbb49d9 to c00f752 Compare December 12, 2023 22:56
@gostkin gostkin force-pushed the egostkin/paima-projected-nft-funnel branch from 8ea365e to 97e3cd5 Compare December 12, 2023 23:09
@gostkin gostkin requested a review from ecioppettini December 12, 2023 23:12
payload: {
ownerAddress: event.ownerAddress != null ? event.ownerAddress : '',

actionTxId: event.actionTxId,
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that games only care about NFTs getting locked and queued to unlock. We don't care if the user ever actually ends up withdrawing their NFT from the lock later.

export const ChainDataExtensionCardanoProjectedNFTConfig = Type.Object({
type: Type.Literal(CdeEntryTypeName.CardanoProjectedNFT),
contract: Type.String(),
scheduledPrefix: Type.String(),
Copy link
Contributor

@SebastienGllmt SebastienGllmt Dec 13, 2023

Choose a reason for hiding this comment

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

Probably this should be optional since most games will probably not need to update their state machine based on somebody locking or queuing to unlock

Probably what is more important is the getCardanoAddressProjectedNfts function you wrote

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 probably don't fully understand the role of scheduledPrefix, could you explain pls?

Copy link
Contributor

Choose a reason for hiding this comment

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

scheduledPrefix is the event that get scheduled to be sent over to the state machine for the game

For example, you can see entrypoint for the state machine of a game here use a switch-case on the prefix of the message it receives from Paima.

scheduledPrefix: Type.String(),
startSlot: Type.Number(),
stopSlot: Type.Optional(Type.Number()),
name: Type.String(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should allow specifying a list of NFT collections you want to filter too. It seems like maybe over-optimizing, but it may cut down on the size of the Carp requests so it may actually make a difference (since if many contracts are using projected NFTs, filtering Carp queries for a specific set of collections may reduce the number of times you have to paginate)

Not a high priority for v1 I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can do it easily, but i'd change asset to policy id and asset name for better search options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,347 @@
/* tslint:disable */
Copy link
Contributor

Choose a reason for hiding this comment

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

don't commit this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's add it to gitignore then

Comment on lines 18 to 20
query(url, Routes.projectedNftEventsRange, {
range: { minSlot: fromAbsoluteSlot, maxSlot: toAbsoluteSlot },
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this handle pagination properly?

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, as long as the range is not too big it will work fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and the request is not split to pages

.gitignore Outdated
.idea/

packages/engine/paima-rest/src/tsoa/routes.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't add this to gitingore I think because it will break some other systems that require parsing this file. The real solution would be trying to find out why this file sometimes gets formatted by a tool (possibly prettier) despite being specified in the .prettierignore. Running prettier on the git repo as expected ignores this file, but it's possible nx or some other tool is not respecting this somewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but how git relates to these systems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's just a file that is not tracked by the repo if it is ignored

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why would it break anything? otherwise one should always specify all subfolders or do git rm on this file which is not very convenient. if we don't need a file in a repo it's better to ignore it imo if it doesn't break things

Copy link
Contributor

@SebastienGllmt SebastienGllmt Dec 21, 2023

Choose a reason for hiding this comment

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

Many systems and tools look at git for their behavior. Please let's not waste time arguing about this and just do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as discussed on the call i added back this file since it was not tracked by git and removed it from gitignore

@SebastienGllmt
Copy link
Contributor

We also need to update the Paima docs with information about this CDE as well

@gostkin
Copy link
Contributor Author

gostkin commented Dec 21, 2023

We also need to update the Paima docs with information about this CDE as well

will add, are the docs blocking this PR to be merged?

…t-asset

Make policy id and asset name separate
@SebastienGllmt SebastienGllmt merged commit 13f2781 into master Dec 29, 2023
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