-
Notifications
You must be signed in to change notification settings - Fork 178
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 chain id #219
fix chain id #219
Conversation
Uxio0
commented
Mar 28, 2022
- Fix chainId for not existing networks on our enum
- Set version 3.9.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't we still have partial functionality in the end?
get_chain_id
would work for any chainget_network
would only work with our enum
Yes, I don't like it either... Do you have any idea? |
I think if the core functionality relies on the supported networks of the enum then I would still use the enum and the chain id still needs to be in the enum. I think it's fine if we say that the networks that we support were part of a review process so it shouldn't work with any The alternative would be to refactor anything to not take the enum into consideration and work with the chain id while taking the metadata that we have right now in the repo as optional. |
Then you are breaking people trying to use this library (or even the safe-cli) on private networks
I disagree, we should try to support every network possible without our intervention.
I think it's alright if we use the |
We are not as this is the current behaviour or was this issue introduced recently? 🤔
That's what I meant: "I think it's fine if we say" – this is to say that depending on the scope of what we want to support then I can understand this change. So if it's clear that we want to support any chain then I think we should say what might not work if the chain is not part of the enum.
Also fine with me. I think it just needs to be clear our level of support because before we would not support chains not available under the enum. |
This is the current behaviour, detected today by checking an issue on the
I updated the docs to make it clear |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in #219 (comment)
For me this change is fine although it seems to be more like a major release in terms of support than an hotfix.
But this is just my personal take on it.
047303f
to
f6aec21
Compare
For me it's only a hotfix. There's no change on any function or signature |