-
Notifications
You must be signed in to change notification settings - Fork 28
feat: add distribution module #181
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
Conversation
You should merge again That said once merged the IBC setup in the e2e tests is still failing, error is :
I wasn't able to find a fix for now... |
@Pantani Some pending conflicts after #141 was updated. @julienrbrt can you also review this when you have time? |
I think the pulsar files can be removed, as Julien did in his branch. |
0141f86
to
ea6d970
Compare
# Conflicts: # .golangci.yml # Makefile # app/keepers/keepers.go # app/keepers/keys.go # app/modules.go # tests/e2e/query_test.go
@@ -0,0 +1,173 @@ | |||
syntax = "proto3"; | |||
package atomone.distribution.v1beta1; |
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.
Since this module is aimed to be part of the fork, is it OK to use the atomone
namespace for it ?
In other words, are we going to replace the cosmos
namespace to atomone
for everything in the fork ?
(for remind, the existing gov module which should also go to the fork already uses the atomone
namespace)
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.
This is also my question. IMHO, as a fork but as a different SDK, it will be nice to have different namespaces. But we can lose some integrations we already have for the cosmos-sdk
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.
Indeed, I would rather keep the original namespace, but for x/gov
it will difficult to change it back so cosmos
, since clients have already integrated the atomone
namespace...
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.
Great, I will return the namespaces to cosmos
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.
If you don't mind I prefer to keep the comment unresolved until the namespace is returned to cosmos
.
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.
This PR will ideally be done in the SDK? So the buf issues shouldn't happen then.
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.
That's a good point @julienrbrt. Initially we have decided to fork the distribution module in atomone, but given this namespace problem and the fact that now atomone already depends on our SDK fork, we should move the fork of the distribution module directly to our SDK fork, with the cosmos
namespace.
@Pantani could you close this PR and make the same on atomone-one/cosmos-sdk
?
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.
Note that for now the atomone gov
module will stay on atomone, because it is more closely tied to the AtomOne Constitution.
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.
Note that for now the atomone gov module will stay on atomone, because it is more closely tied to the AtomOne Constitution.
We could port most of the changes to the SDK fork, and keep atom one gov module (for client compat) in atom one as a simple wrapper instead of fork.
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.
Ah yes I remember you told me about this, let's keep that in mind.
# Conflicts: # .golangci.yml # Makefile
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.
Looks good, but as commented we need to keep the original namespace for protos, because introducing a new one will break client integrations. And considering this module will end up in our fork, this is an other good reason to keep the cosmos
namespace.
close because we move this change to atomone cosmos-sdk |
Add the distribution module from
v0.50.13
with some cleanups