Skip to content

Conversation

@shargon
Copy link
Member

@shargon shargon commented Oct 1, 2025

Description

Please @neo-project/core review this PR, as we agree in Centre Point it's important for the community.
Was decided in Centre Point to have two decimals in ExecutionFactor this PR tries to address it

Type of change

  • Optimization (the change is only an optimization)
  • Style (the change is only a code style for better maintenance or standard purpose)
  • 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

How Has This Been Tested?

  • Test A
  • Test B

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

@roman-khimov roman-khimov mentioned this pull request Oct 1, 2025
15 tasks
@shargon shargon marked this pull request as ready for review October 2, 2025 07:50
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.

Much cleaner now. But I'd also adjust BurnGas() since it's documented to use datoshis and for its users (if there are any) that should be sufficient.

@shargon shargon changed the title Exec Factor with two decimals Exec Factor with decimals Oct 3, 2025
@shargon
Copy link
Member Author

shargon commented Oct 3, 2025

I think it could be more clear if we multiply inside AddFee method, otherwise, AddFee it's in picoDatoshi and BurnFee it's in datoshi, what do you think @roman-khimov @AnnaShaleva?

@roman-khimov
Copy link
Contributor

AddFee is an internal thing, BurnFee is only used by contracts. To me it's not a problem.

@shargon shargon mentioned this pull request Oct 3, 2025
15 tasks
@shargon
Copy link
Member Author

shargon commented Oct 6, 2025

Please @neo-project/core review this PR, as we agree in Centre Point it's important for the community.

Important changes areas:

  • AddFee should use picoDatoshi always

@shargon shargon added the Critical Issues (bugs) that need to be fixed ASAP label Oct 6, 2025
@shargon shargon added Help Wanted We suggest a careful read at https://docs.neo.org/. However, fell free to further discuss the topic. and removed Work in Progress labels Oct 6, 2025
@roman-khimov
Copy link
Contributor

AddFee should use picoDatoshi always

picoGAS! It's 8+4, so 1^-12. picoDatoshi would be 1^-20.

@Wi1l-B0t
Copy link
Contributor

Wi1l-B0t commented Oct 29, 2025

The gasconsumed in Application Log before HF_Faun is Datoshi.

The gasconsumed in Application Log after HF_Faun is Datoshi or PicoGAS?

@shargon shargon dismissed stale reviews from ajara87 and Wi1l-B0t via 9fa5998 October 29, 2025 11:58
@shargon
Copy link
Member Author

shargon commented Oct 29, 2025

Thanks @superboyiii ! please check it again, it should be fixed

@shargon
Copy link
Member Author

shargon commented Oct 29, 2025

he gasconsumed in Application Log is Datoshi or PicoGAS?

should not be changed outside the ApplicationEngine

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.

Let me check it with NeoGo implementation.

ajara87
ajara87 previously approved these changes Oct 29, 2025
ajara87
ajara87 previously approved these changes Oct 29, 2025
@superboyiii
Copy link
Member

I tried some blocks, no incompatible data found. Let me make a full sync to check all.

@superboyiii
Copy link
Member

Storage is compatible now.

Wi1l-B0t
Wi1l-B0t previously approved these changes Nov 4, 2025
@shargon shargon dismissed stale reviews from Wi1l-B0t and ajara87 via 58d39a1 November 5, 2025 10:19
@shargon shargon mentioned this pull request Nov 5, 2025
18 tasks
@shargon
Copy link
Member Author

shargon commented Nov 5, 2025

Closed in favor of #4278

  • user branch
  • rebased from master (lot of conflicts)

@shargon shargon closed this Nov 5, 2025
@erikzhang erikzhang deleted the exec-fee-two-decimals branch November 5, 2025 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Critical Issues (bugs) that need to be fixed ASAP Need Testing Waiting for Review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants