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

Adding Player Handles #136

Closed
wants to merge 1 commit into from
Closed

Adding Player Handles #136

wants to merge 1 commit into from

Conversation

faytey
Copy link
Contributor

@faytey faytey commented May 20, 2024

Context (Problem, Motivation, Solution)

Link related issues!
Issue 117

Describe Your Changes

I added the smart contract to implement Player handles and wrote the tests to check the functions work properly

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.

@faytey
Copy link
Contributor Author

faytey commented May 20, 2024

I need your review on this before proceeding @norswap , Also I'm encountering errors setting up the webapp

@faytey
Copy link
Contributor Author

faytey commented May 28, 2024

I need your review on this before proceeding @norswap , Also I'm encountering errors setting up the webapp

Hello @norswap still awaiting your review

Copy link
Member

@norswap norswap left a comment

Choose a reason for hiding this comment

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

Great work! Sorry again for the delay. Could you make the requested changes? I'll make sure to prioritize reviewing them now.

package-lock.json Outdated Show resolved Hide resolved
packages/contracts/src/PlayerHandle.sol Outdated Show resolved Hide resolved
packages/contracts/src/PlayerHandle.sol Outdated Show resolved Hide resolved
if (b.length < 5) return false;
for (uint256 i; i < b.length; i++) {
bytes1 char = b[i];
if (char < 0x20 || char > 0x7E || char == 0x2E) {
Copy link
Member

Choose a reason for hiding this comment

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

Note to myself: will probably want to change this to only support alphanumerics & dashes, underscores, square brackets (for cute clan names), spaces (but not more than one in a row, and not at the end or at the start of the handle), as well as put a max character limit.

Fine to leave this as is for this PR.

packages/contracts/src/test/PlayerHandle.t.sol Outdated Show resolved Hide resolved
@faytey
Copy link
Contributor Author

faytey commented Jul 1, 2024

Hi Sir I'd get started on these and update, sorry for the delay

@faytey
Copy link
Contributor Author

faytey commented Jul 3, 2024

Hi Sir, I have made requested corrections

@norswap
Copy link
Member

norswap commented Jul 3, 2024

Great work! Could you just make sure the Mock ENS resolver is logged + included in deployments.ts as well?

@norswap
Copy link
Member

norswap commented Jul 3, 2024

Also it seems you've made changes to the lockfile, could just revert it to the master version so that no changes show? The current file seems incorrect in some way, as shown by the failing CI check.

@faytey
Copy link
Contributor Author

faytey commented Jul 3, 2024

Also it seems you've made changes to the lockfile, could just revert it to the master version so that no changes show? The current file seems incorrect in some way, as shown by the failing CI check.

Hi Sir, I didn't make any changes to the lockfile and can't seem to understand how to rectify it.

@norswap
Copy link
Member

norswap commented Jul 3, 2024

That was probably accidental, you can see it is changed in "Files changes" here on Github.

Fixing it requires a little bit of git foo:
You can revert it by using git checkout master -- pnpm-lock.yaml then git add pnpm-lock.yaml.
You could then git commit and push that, but let's squash all your commit together:
git reset --soft HEAD~7 && git commit

(The 7 here is the number of commits you've made, so if you add additional commits in between you need to update that number.)

Then finally git push --force-with-lease to replace the commits here with the single squashed commit.

@faytey
Copy link
Contributor Author

faytey commented Jul 3, 2024

That was probably accidental, you can see it is changed in "Files changes" here on Github.

Fixing it requires a little bit of git foo: You can revert it by using git checkout master -- pnpm-lock.yaml then git add pnpm-lock.yaml. You could then git commit and push that, but let's squash all your commit together: git reset --soft HEAD~7 && git commit

(The 7 here is the number of commits you've made, so if you add additional commits in between you need to update that number.)

Then finally git push --force-with-lease to replace the commits here with the single squashed commit.

done sir

@faytey
Copy link
Contributor Author

faytey commented Jul 6, 2024

Hello sir @norswap

@norswap
Copy link
Member

norswap commented Jul 21, 2024

Thanks! Did some extra cleanup and merged here: #137

@norswap norswap closed this Jul 21, 2024
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