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

feat(core): Add coin definition for eCash XEC (altcoin based on bitcoin) #4336

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

Conversation

Fabcien
Copy link
Contributor

@Fabcien Fabcien commented Nov 11, 2024

This PR adds the definition for eCash (XEC).

I tried to use support.py to generate the support.json but this currenly fails due to the missing release file for T3W1 so I updated manually.

make gen builds successfully. I also ran make style_check but this failed on me with 122 reportArgumentType errors from pyright. I'm probably not using the right version, but all the reports are unrelated to this change, and the other linters return no error. I expect it to pass on CI.

@Fabcien
Copy link
Contributor Author

Fabcien commented Nov 13, 2024

@matejcik Can you explain why the CI is failing and if there is something I need to do to fix it ? It looks like a permission issue at first sight but not sure

@matejcik
Copy link
Contributor

"add to github project" -> don't worry about this one

"gen check" -> you did not run make gen and commit the resulting changes
"defs check" -> for some reason our checker requires coin_name and coin_label to start with an uppercase letter. i believe the right thing to do is add an exception for e to the regexps in common/tools/coin_info.py

@matejcik
Copy link
Contributor

I also ran make style_check but this failed on me wi

export PYRIGHT_PYTHON_FORCE_VERSION=1.1.361

@Fabcien
Copy link
Contributor Author

Fabcien commented Nov 13, 2024

I pushed 2 new commits:

  • One to add an exception for defs_checks as suggested by @matejcik
  • One to include the output of make gen

I ran make style_check (thanks for the hint), make gen_check and make defs_check and they all passed.

Should I squash the commits ?

@@ -0,0 +1,44 @@
{
"coin_name": "eCash",
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to keep the name like this.

That way we do not need to change 2 regex-es, only one.

Suggested change
"coin_name": "eCash",
"coin_name": "Ecash",

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 prefer to keep the name like this.

That way we do not need to change 2 regex-es, only one.

Good call, since it's not used for display it's fine to keep the uppercase here. I updated and pushed accordingly.

@Fabcien
Copy link
Contributor Author

Fabcien commented Nov 14, 2024

@matejcik @prusnak Is there anything else I need to do ?

@matejcik
Copy link
Contributor

the support info is wrong. T1B1 should be 1.12.2 (next version up), all others should be 2.8.6 (next version up)
also please fixup the "make gen" commit into the "add coin definition", and update the other commit to use ConventionalCommit name (feat(common):)

The regex enforces starting with an uppercase letter, which doesn't work for eCash and for xRhodium. An exception is added to handle this naming, as well as a comment to remind why the regex contains this special case.
This fixes `make defs_check` for eCash.
@Fabcien
Copy link
Contributor Author

Fabcien commented Nov 14, 2024

@matejcik Thanks I updated accordingly.

@Fabcien
Copy link
Contributor Author

Fabcien commented Nov 15, 2024

@matejcik @prusnak Is there anything left to do ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔎 Needs review
Development

Successfully merging this pull request may close these issues.

3 participants