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

Fix an issue where currency does not work correctly in dotnet6 #3880

Merged
merged 2 commits into from
Jul 17, 2024

Conversation

s2quake
Copy link
Contributor

@s2quake s2quake commented Jul 15, 2024

In dotnet6, the following error occurs when building.

CS0188, CS8885: The this object cannot be used before all of its fields are assigned to

I tried to change the Hash field of Currency to a property, but it was not the correct way to modify it.

According to the error message, I just need to avoid calling this method, so I changed the method to static and used it.

The code is less readable, but I was able to solve the problem for now.

@s2quake s2quake force-pushed the fix/currency-dotnet6 branch from 2aa539e to ea9808e Compare July 15, 2024 09:09
@s2quake s2quake changed the title Fix an issue whrer currency does not work correctly in dotnet6 Fix an issue where currency does not work correctly in dotnet6 Jul 16, 2024
@s2quake s2quake requested a review from greymistcube July 16, 2024 02:01
@s2quake s2quake marked this pull request as ready for review July 16, 2024 02:05
@s2quake s2quake requested a review from riemannulus July 16, 2024 02:06
Comment on lines 731 to 736
private static IValue SerializeForHash(
IImmutableSet<Address>? minters,
string ticker,
byte decimalPlaces,
(BigInteger Major, BigInteger Minor)? maximumSupply,
bool totalSupplyTrackable)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary?

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 think it's not, That code has been moved into GetHash.
amended 997922b

Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't there a reason why GetHash() and SerializeForHash() were separate in the first place? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, nevermind, it was about SerializeForHash() and Serialize(). 🙄

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be good to have a simple comment on whether we have a separate serialization scheme. 😐

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be good to have a simple comment on whether we have a separate serialization scheme. 😐

3808b22#diff-7975b57b58acd250c0c5fb4f0001c98e298746481661b38ccb8e64c429dda7b5R731
How about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment doesn't clarify as to why the Dictionary construction is different from Serialize(), which I think would be the main point of confusion for those seeing the code without context. I'd say the history regarding GetHash() is barely relevant and doesn't need mentioning.

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 comment doesn't clarify as to why the Dictionary construction is different from Serialize(), which I think would be the main point of confusion for those seeing the code without context. I'd say the history regarding GetHash() is barely relevant and doesn't need mentioning.

9a8fd89#diff-7975b57b58acd250c0c5fb4f0001c98e298746481661b38ccb8e64c429dda7b5R731
amended

@s2quake s2quake force-pushed the fix/currency-dotnet6 branch 2 times, most recently from 6b9b285 to 5298df7 Compare July 16, 2024 04:40
@s2quake s2quake requested a review from riemannulus July 16, 2024 05:07
@s2quake s2quake force-pushed the fix/currency-dotnet6 branch from 5298df7 to b9064f8 Compare July 17, 2024 00:51
@s2quake s2quake force-pushed the fix/currency-dotnet6 branch from b9064f8 to a9f0615 Compare July 17, 2024 04:07
@riemannulus riemannulus merged commit 63db1a5 into 5.1-maintenance Jul 17, 2024
30 checks passed
@riemannulus riemannulus deleted the fix/currency-dotnet6 branch July 17, 2024 05:38
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