Skip to content

use datoshi as the name of the smallest unit of GAS#3241

Merged
NGDAdmin merged 9 commits intoneo-project:masterfrom
Jim8y:use-datoshi
May 27, 2024
Merged

use datoshi as the name of the smallest unit of GAS#3241
NGDAdmin merged 9 commits intoneo-project:masterfrom
Jim8y:use-datoshi

Conversation

@Jim8y
Copy link
Contributor

@Jim8y Jim8y commented May 19, 2024

Description

It is ambiguous to call both the smallest unit of GAS and GAS as GAS, thus i had started a discussion and vote in neo discord, and get a name of datoshi for the smallest unit of GAS. This name has being approved by NF. Thus having this pr.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Test Configuration:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@Jim8y
Copy link
Contributor Author

Jim8y commented May 19, 2024

Datoshi is the name for the smallest unit of GAS.

1 datoshi = 1e-8 GAS
1 kdatoshi = 1e-5 GAS
1 mdatoshi = 1e-2 GAS

Or

1_00_000_000 datoshi = 1_00_000 kdatoshi = 1_00 mdatoshi = 1 GAS

cschuchardt88
cschuchardt88 previously approved these changes May 19, 2024
Copy link
Member

@cschuchardt88 cschuchardt88 left a comment

Choose a reason for hiding this comment

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

what is this new datoshi thing? Just gas?

@Jim8y
Copy link
Contributor Author

Jim8y commented May 20, 2024

what is this new datoshi thing? Just gas?

Yes, just give the smallest unit of GAS a name, cause people including our own are miss using GAS and the smallest unit of GAS, such as 20 GAS and 20_0000_0000 GAS.

@Jim8y Jim8y changed the title use datoshi as the smallest unit of GAS use datoshi as the name of the smallest unit of GAS May 20, 2024
@superboyiii
Copy link
Member

Does DA know this :)

@Jim8y
Copy link
Contributor Author

Jim8y commented May 20, 2024

Does DA know this :)

yes

@shargon
Copy link
Member

shargon commented May 20, 2024

The problem is GAS not satoshi, isn't it?

@Jim8y
Copy link
Contributor Author

Jim8y commented May 20, 2024

The problem is GAS not satoshi, isn't it?

@shargon The problem is we dont have a name for the smallest unit of GAS while we are actually use it a lot. And in the core, sometimes we use GAS for GAS, sometimes we use the smallest GAS unit as GAS. Its misused.

{
GasConsumed = checked(GasConsumed + gas);
if (GasConsumed > gas_amount)
FeeConsumed = GasConsumed = checked(FeeConsumed + datoshi);
Copy link
Member

Choose a reason for hiding this comment

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

@Jim8y please remove the warning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warning is expected, marking and telling people that this is absolete. If we disable it here, people how use this will also not able to see it.

Copy link
Member

Choose a reason for hiding this comment

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

But the warning will apif people use it, is not for us

Copy link
Contributor Author

@Jim8y Jim8y May 20, 2024

Choose a reason for hiding this comment

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

The warning shows to people exactly the way it shows us, during the build process, if we disable it here, it will also go disappear in users build terminal..... can be done easily, but i am not sure you like that.

Copy link
Contributor

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Two obligatory links:

And one minor note: I always expect users to operate with GAS, so things like "please input extra fee" SHOULD NOT accept satoshi values, they should just use 0.xxx GAS. At least that's the way NeoGo CLI is.

@Jim8y
Copy link
Contributor Author

Jim8y commented May 20, 2024

Its voting result. Fixed8 makes too much change to the existing code. And use 0.xxxGAS is good but will change existing neo-cli, a thing should be avoided in this pr.

shargon
shargon previously approved these changes May 21, 2024
Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

I still prefer the old way of GAS/satoshi identification, but if it's the user's decision, then let's keep it.

@AnnaShaleva
Copy link
Member

I think there's a couple of places in the neo-devpack that needs to be updated as far.

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

@Jim8y, let's wait until coverage is recovered in PR #3252.

Then, we can add this safely with new UTs and correct tests for coverage.

@vncoelho vncoelho dismissed their stale review May 22, 2024 01:34

Not a blocker

cschuchardt88
cschuchardt88 previously approved these changes May 23, 2024
@Jim8y
Copy link
Contributor Author

Jim8y commented May 23, 2024

ready to merge

AnnaShaleva
AnnaShaleva previously approved these changes May 23, 2024
@shargon
Copy link
Member

shargon commented May 24, 2024

@Jim8y maybe now with the mono-repo we need more changes, did you check it?

@Jim8y
Copy link
Contributor Author

Jim8y commented May 24, 2024

I will check it now.

@Jim8y Jim8y dismissed stale reviews from AnnaShaleva, cschuchardt88, and shargon via b8656ff May 25, 2024 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants