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

New voucher name and symbol #477

Merged
merged 6 commits into from
Dec 6, 2022
Merged

New voucher name and symbol #477

merged 6 commits into from
Dec 6, 2022

Conversation

zajck
Copy link
Member

@zajck zajck commented Nov 29, 2022

Change voucher name in symbol to

  • name: Boson Voucher (rNFT)
  • symbol: BOSON_VOUCHER_RNFT

@zajck zajck force-pushed the rename-voucher-constants branch from a50a327 to 7b85376 Compare November 29, 2022 07:35
Copy link
Contributor

@anajuliabit anajuliabit left a comment

Choose a reason for hiding this comment

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

Could you please also add (rNFT) to docs/architeture.md#Voucher NFT

@cliffhall
Copy link
Contributor

I thought we were going to limit the term rNFT's incursion into the codebase by only changing the token name and symbol. This additional change of references in code and Ana's request to change the architecture.md go beyond that. This is the slippery slope. You have to draw a line somewhere and the further in you go with it, the harder it is to draw a clean line.

@anajuliabit
Copy link
Contributor

anajuliabit commented Nov 30, 2022

@cliffhall the changes that @zajck made on the docs are on places that we had called Voucher NFT. Voucher rNFT better suits the current version of the protocol IMO

anajuliabit
anajuliabit previously approved these changes Nov 30, 2022
@zajck
Copy link
Member Author

zajck commented Nov 30, 2022

I thought we were going to limit the term rNFT's incursion into the codebase by only changing the token name and symbol. This additional change of references in code and Ana's request to change the architecture.md go beyond that. This is the slippery slope. You have to draw a line somewhere and the further in you go with it, the harder it is to draw a clean line.

That;s what I did at first. Then after @anajuliabit's comment I made also some changes to code, but only where "NFT" already existed.

But now, giving it another though, I think that we should completely remove "NFT" or "rNFT" from the code and also from docs/architecture.md. My rationale:

  • "Voucher" and "rNFT" are interchangeable, so it's enough to use only one of them (in our case voucher)
  • Wherever "NFT" was still in the comments, it was more of a legacy thing. It may even lead to a confusion if voucher and NFT are two things.

We could say that "Voucher" represents the implementation. And "rNFT" is only a name that we use for our use case. If someone else deploys complete protocol, they are completely free to not use "rNFT". That's whay I would remove it everywhere.

@anajuliabit
Copy link
Contributor

@zajck You have a point. I agree!

Copy link
Contributor

@anajuliabit anajuliabit left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@anajuliabit anajuliabit left a comment

Choose a reason for hiding this comment

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

LGTM

@zajck zajck requested review from cliffhall and mischat December 2, 2022 09:15
Copy link
Contributor

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@zajck zajck merged commit 2af8b44 into main Dec 6, 2022
@zajck zajck deleted the rename-voucher-constants branch December 6, 2022 07:13
@anajuliabit anajuliabit mentioned this pull request Dec 30, 2022
38 tasks
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