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

[Neo Fix] fix the store crash issue #3124

Merged
merged 4 commits into from
Feb 8, 2024
Merged

Conversation

Jim8y
Copy link
Contributor

@Jim8y Jim8y commented Feb 8, 2024

Description

This pr focus on fixing the issue mentioned in #3123 that the default value of Factory and the initialization order of NeoSystem and StoreFactory caused the null reference.

Fixes # (issue) #3123

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

How Has This Been Tested?

Run existing tests.

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 Jim8y linked an issue Feb 8, 2024 that may be closed by this pull request
src/Neo/NeoSystem.cs Outdated Show resolved Hide resolved
@Jim8y
Copy link
Contributor Author

Jim8y commented Feb 8, 2024

Updated @shargon

@shargon shargon merged commit 95708b5 into neo-project:master Feb 8, 2024
1 of 2 checks passed
if (_neoSystem != null) return;
if (NeoSystem != null) return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shargon why revert these back to NeoSystem? this code is useless here, you must use _neoSystem.

@Jim8y Jim8y deleted the fix-store branch February 8, 2024 11:18
{
return StoreFactory.GetStore(store.GetType().Name, path);
}

Copy link
Member

Choose a reason for hiding this comment

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

why did you remove this? This will break plugins

Copy link
Member

Choose a reason for hiding this comment

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

Mark as obsolete

Copy link
Contributor Author

@Jim8y Jim8y Feb 8, 2024

Choose a reason for hiding this comment

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

#3126 now check it here, wont mark it as absolete, still using it. My bad. Need some rest to work more carefully.

Copy link
Member

Choose a reason for hiding this comment

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

Its an public API should be marked as obsolete. If you want to remove it.

Copy link
Member

Choose a reason for hiding this comment

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

We can use Factory in modules, it's not a problem, currently only neo use neo library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use Factory in modules, it's not a problem, currently only neo use neo library.

No, I have searched, neoxp also use it, not sure any other tools also using it. @cschuchardt88 is right, its public, cant be changed easily.

Jim8y added a commit to Jim8y/neo that referenced this pull request Feb 9, 2024
* 'nullable' of github.com:Jim8y/neo:
  [Neo Fix] fix the store crash issue (neo-project#3124)
  [Neo VM: FIX] the GetString() methods of bytestring requires strictUTF8 (neo-project#3110)
@roman-khimov roman-khimov mentioned this pull request Feb 15, 2024
12 tasks
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.

Problems running current master version
3 participants