Use type accAddress instead of hash.Hash for resources' hash #1704
Conversation
NicolasMahe
changed the title
|
|
||
| func TestHashSerialize(t *testing.T) { | ||
| require.Equal(t, "2:cosmos1ffzdc9fkggz2srlgp6grj32uc9sg9qvzu4jym8;3:eventKey;4:1:foo:3:bar;;;;", data.HashSerialize()) | ||
| require.Equal(t, "cosmos186j7ddz0046caf63h566ez5nxd23svd44ga94h", sdk.AccAddress(crypto.AddressHash([]byte(data.HashSerialize()))).String()) |
There was a problem hiding this comment.
by default, the bech32 are using cosmos prefix.
we override it with:
cfg, err := config.New()
if err != nil {
panic(err)
}
cosmos.CustomizeConfig(cfg)I would like to extract the prefix from the config and hardcode them somewhere so we could override the cosmos's prefix in an init function.
# Conflicts: # hash/hash_test.go # service/service.pb.go
There was a problem hiding this comment.
I like the idea a few feedaback:
- some
validate:"required,accaddress"are missing in the proto, might be good to add them just in case. - remove the
equal_allin proto, I'm ok but we need to make sure that we really don't use it.
In general I love this PR, one thing tho, Cosmos is migrating all its types in proto for the next release so I would wait for that, instead of breaking the API to break it again on the next cosmos version, let's wait for the next cosmos version and do all this work just once
| e := &Event{ | ||
| InstanceHash: instanceHash, | ||
| Key: eventKey, | ||
| Data: eventData, | ||
| } | ||
| e.Hash = hash.Dump(e) | ||
| e.Hash = sdk.AccAddress(crypto.AddressHash([]byte(e.HashSerialize()))) |
There was a problem hiding this comment.
events might not be necessary but for the sake of normalization ok :)
| @@ -45,20 +47,22 @@ func (b *Builder) Create(req *api.CreateRunnerRequest) (*runnerpb.Runner, error) | |||
| } | |||
|
|
|||
| instanceEnv := xos.EnvMergeSlices(srv.Configuration.Env, req.Env) | |||
| envHash := hash.Dump(hashserializer.StringSlice(instanceEnv)) | |||
| envHash := hash.Sum([]byte(strings.Join(instanceEnv, ","))) | |||
There was a problem hiding this comment.
why not doing the same here too ;)
this way we only have one hashing function, same that events that are not needed as a cosmos address but not bad representation anyway
| @@ -268,9 +268,9 @@ func convertVolumesFrom(s *service.Service, dVolumesFrom []string) ([]container. | |||
| // volumeKey creates a key for service's volume based on the sid to make sure that the volume | |||
| // will stay the same for different versions of the service. | |||
| func volumeKey(s *service.Service, dependency, volume string) string { | |||
| return hash.Dump(hashserializer.StringSlice([]string{ | |||
| return hash.Sum([]byte(strings.Join([]string{ | |||
There was a problem hiding this comment.
here too, cosmos address can be use
| runnerAddress := sdk.AccAddress(crypto.AddressHash(run.Hash)) | ||
| serviceAddress := sdk.AccAddress(crypto.AddressHash(srv.Hash)) | ||
| if err := k.distributePriceShares(ctx, execAddress, runnerAddress, serviceAddress, exec.Price); err != nil { | ||
| if err := k.distributePriceShares(ctx, exec.Hash, run.Hash, srv.Hash, exec.Price); err != nil { |
|
closing but discussion continue here: #1715 |
POC of replacing resource's
hash.HashbyAccAddress.