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

chore: Synchronize contract interface naming #2801

Merged
merged 2 commits into from
Aug 30, 2023

Conversation

howjmay
Copy link
Member

@howjmay howjmay commented Aug 15, 2023

No description provided.

@howjmay
Copy link
Member Author

howjmay commented Aug 15, 2023

What is the rule of the naming of the keys of state variables, parameters, prefixes? Maybe we need one?

@howjmay howjmay force-pushed the core-contract-naming branch 4 times, most recently from 1e56e15 to 0d4251c Compare August 15, 2023 18:07
@howjmay howjmay marked this pull request as ready for review August 16, 2023 00:03
@howjmay howjmay force-pushed the core-contract-naming branch 7 times, most recently from aa0340b to 368c42d Compare August 22, 2023 16:15
Copy link
Contributor

@jorgemmsilva jorgemmsilva 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 the rule of the naming of the keys of state variables, parameters, prefixes? Maybe we need one?

we don't have anything strict, basically "don't repeat vars" and "single char as most as possible to save space". To be honest I don't like how we do it tho, using an enum or similar would probably be nicer so we cannot have human error creating collision between keys in the same state. But it is what it is, it works in the current form and we should be changing this much in the future anyway


This is a positive change, its good to keep the code with some organization/structure.
however, I've noticed these changes result in the testdbhash being updated, which it shouldn't IMO

@howjmay
Copy link
Member Author

howjmay commented Aug 29, 2023

What is the rule of the naming of the keys of state variables, parameters, prefixes? Maybe we need one?

we don't have anything strict, basically "don't repeat vars" and "single char as most as possible to save space". To be honest I don't like how we do it tho, using an enum or similar would probably be nicer so we cannot have human error creating collision between keys in the same state. But it is what it is, it works in the current form and we should be changing this much in the future anyway

This is a positive change, its good to keep the code with some organization/structure. however, I've noticed these changes result in the testdbhash being updated, which it shouldn't IMO

@jorgemmsilva
But I changed the naming of variables in this PR, which would cause the binary is not the same. I think the db hash should changed, since we save the binary to kvstore.
Not sure whether I made some mistake for the understanding

@jorgemmsilva
Copy link
Contributor

@howjmay

But I changed the naming of variables in this PR, which would cause the binary is not the same. I think the db hash should changed, since we save the binary to kvstore.

exactly. But that should not happen (we shouldn't change the keys as it will cause issues on running networks)

@howjmay
Copy link
Member Author

howjmay commented Aug 29, 2023

@jorgemmsilva
So change like this I should discard in this PR?
https://github.com/iotaledger/wasp/pull/2801/files#diff-d006fa22dfaf47a4bd1d3856d50e1b301921108506448973993232694853c3c1R62

I think this place could be one of the reason that I need to update the hash

@howjmay howjmay merged commit d6047ce into iotaledger:develop Aug 30, 2023
5 checks passed
@howjmay howjmay deleted the core-contract-naming branch August 30, 2023 09:24
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.

2 participants