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

Decks Onchain #134

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Decks Onchain #134

wants to merge 24 commits into from

Conversation

eviterin
Copy link
Collaborator

@eviterin eviterin commented Mar 8, 2024

Context (Problem, Motivation, Solution)

#103 Bring Decks Onchain
#111 Add Deck Validity Status when Constructing Deck

Describe Your Changes

Decks are now saved and modified onchain.
Decks owned by the player are now presented in their collection.
Minor changes to UI but not overhauled (#93)

A couple of functions added in the Inventory.sol contract, namely:
getAllDecks
getDeckNames
getDeckReal

Checklist

  • I have performed a self-review of my code
  • I ran make check and fixed resulting issues
  • I ran the relevant tests and they all pass
  • I wrote tests for my new features, or added regression tests for the bug I fixed

Testing

If you didn't write tests, explain how you made sure the code was correct and working as intended.

@eviterin
Copy link
Collaborator Author

eviterin commented Mar 8, 2024

This one should be in draft state, but I can't set it to that.
Still have tests & checks to do, and refactor getDeckReal as it clashes with a function named getDeck that fetches only the cards of a deck(not name).

@norswap
Copy link
Member

norswap commented Mar 12, 2024

You don't see "Still in progress? Convert to Draft" in the top right panel?
If you do see it, what happens if you click it?

You have write access to the repo, so you should be able to convert the PR to draft in theory!

Do you want me to already give this a review, or do you want me to wait when you're done?

@eviterin eviterin marked this pull request as draft March 12, 2024 20:29
@eviterin eviterin force-pushed the DeckChainy branch 2 times, most recently from cde5752 to d112e58 Compare March 16, 2024 15:47
@eviterin eviterin requested a review from norswap March 16, 2024 15:53
@eviterin eviterin marked this pull request as ready for review March 16, 2024 15:54
@norswap
Copy link
Member

norswap commented Mar 17, 2024

I see this still has conflict, so should be rebased on top of master!

@norswap
Copy link
Member

norswap commented Mar 17, 2024

It's kind of weird there are commits from Aritra in the middle ... rebase gone wrong?

@eviterin eviterin force-pushed the DeckChainy branch 2 times, most recently from abf2e76 to fa9c55d Compare March 26, 2024 19:33
@eviterin
Copy link
Collaborator Author

eviterin commented Mar 26, 2024

Aritra commits are DELTED.
REBASED on top of master.
All checks PASSING.
Commit history tidied up (some).

I haven't been able to verify if my work has regressed gameplay due to "#135 Unable to Join Game", and am worried as I have refactored getDeck in Inventory.sol that is used there (commit f812094).

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