Fix!: SlashAcks api change #1163
Conversation
|
Hi @smarshall-spitzbart, if you can have a look if this is correct ? Thank you! |
Co-authored-by: Shawn <[email protected]>
Co-authored-by: Shawn <[email protected]>
|
the api change will convert will deal with the covertion |
x/ccv/provider/keeper/keeper.go
Outdated
| consAddresses := []types.ConsumerConsAddress{} | ||
| for _, address := range addresses { | ||
| // reverse of ConsumerConsAddress.String() | ||
| consAddr, err := sdk.ConsAddressFromBech32(address) |
There was a problem hiding this comment.
if someone can help confirmsdk.ConsAddressFromBech32() is the reverse of ConsumerConsAddress.String() ?
There was a problem hiding this comment.
see how consumer handles this
. Although I'm not sure that we need to add the complexity of converting these types back and forth so much
|
|
||
| acks, err := StrToConsumerConsAddresses(addresses) | ||
| if err != nil { | ||
| // todo |
There was a problem hiding this comment.
shall return err ? if yes, where to deal with the err ? @smarshall-spitzbart
There was a problem hiding this comment.
I'd say pattern match other places in the repo that panic or return upon error. See above on how consumer does this
There was a problem hiding this comment.
I'd say we should do one of the two following solutions:
- Only change API of the append function and keep everything else as strings. This is most simple, avoids state migration, but doesn't add much type safety. This solution is just fine if you wanna take this route.
- Store slash acks as the cons addr type, adjust APIs accordingly, only convert to strings one time when the slash acks are about to be sent over wire. This would require migration as it's state breaking
We should avoid converting slash acks back and forth in-mem as that adds unneeded complexity
Co-authored-by: Shawn <[email protected]>
|
Hey these changes have started to look like more work than we thought, lets just close the issue and call it good |
Description
Closes: #728
Remove invalid todos.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...