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

build: update most recent proto files #150

Closed
wants to merge 3 commits into from

Conversation

peterpeterparker
Copy link
Member

@peterpeterparker peterpeterparker commented Jul 19, 2022

Motivation

Update most recent governance, base_types, common types, ledger and sns_swap proto files

TODO

@lmuntaner : as you will notice, some neurons features do not compile anymore with the most recent definition. should we use these last definitions and if yes, can you update the code accordingly?

Changes

  • update proto files and generate related .js and .ts files
  • split base_types in base_types and (common) types as in in ic repo
  • docs "how to update proto files"

@lmuntaner
Copy link
Contributor

@peterpeterparker I don't know where the ledger proto files are, ask Mario Pastorelli on Slack, he will know.

I'd say we should use the last definitions, yes. But we should check with the nns dapp that all the hardware wallet features still work.

I can see when I have a moment and update the code. How urgent is this?

@peterpeterparker
Copy link
Member Author

Not urgent in my opinion.

Only concern I have is the fact that "list neurons" does not match the new proto definition - i.e. needs development to be compatible. So not sure what it means, is that a breaking change? And if yes, is the hw still using an old version of the proto or the new version version as well? Can we actually ugrade?

So yes would not rush except if necessary.

@peterpeterparker
Copy link
Member Author

Proto ledger udpated as well

@lmuntaner
Copy link
Contributor

The current library shows the neurons of the hardware wallet, therefore, (somehow) it still works. That means that it's backwards compatible I guess.

We need to test the changes and make sure they work with the hardware wallet.

I will add it to my todos and take a look when I have some time.

@peterpeterparker
Copy link
Member Author

Thx!

@peterpeterparker
Copy link
Member Author

We just gonna get rid of proto ultimatelly so since this draft PR is 1 year old, let's just close it.

@peterpeterparker peterpeterparker deleted the build/update-most-recent-proto-commits branch July 7, 2023 09:57
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.

None yet

2 participants