remove stride address #1010
remove stride address #1010
Conversation
There was a problem hiding this comment.
Lgtm! Thanks for adding such detailed comments!
I do wonder if the best long term solution is to just remove the receiver altogether with autopilot and use a module address instead (in which case we'd need to bring back the StrideAddress for claim). But that's clearly not something we'd do before the upgrade so I think this is fine for now
| } | ||
|
|
||
| // ibc-go v5 has a Memo field that can store forwarding info | ||
| // For older version of ibc-go, the data must be stored in the receiver field | ||
| // TODO: can we remove this backwards compatibility? |
There was a problem hiding this comment.
gaia's still on ibc v3 I believe.
Are you suggesting we just always use the receiver instead of the memo field?
There was a problem hiding this comment.
I'm suggesting we always use the memo field, never the receiver (putting the metadata in the receiver field seems hacky)
There was a problem hiding this comment.
Agreed it is hacky, but I don't think we have a choice until gaia upgrades
| @@ -125,33 +125,34 @@ func (im IBCModule) OnRecvPacket( | |||
| packet.Sequence, packet.SourcePort, packet.SourceChannel, packet.DestinationPort, packet.DestinationChannel)) | |||
|
|
|||
| // NOTE: acknowledgement will be written synchronously during IBC handler execution. | |||
| var data transfertypes.FungibleTokenPacketData | |||
| if err := transfertypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { | |||
| var fungibleTokenPacketData transfertypes.FungibleTokenPacketData | |||
There was a problem hiding this comment.
nit: these variable names are pretty long and I'm not sure the word fungible adds much to the understanding of this function. Might make sense to rename to just tokenPacketData or packetData
| @@ -89,13 +78,11 @@ func ParsePacketMetadata(metadata string) (*PacketForwardMetadata, error) { | |||
| var routingInfo ModuleRoutingInfo | |||
| if raw.Autopilot.Stakeibc != nil { | |||
| // override the stride address with the receiver address | |||
| @@ -89,13 +78,11 @@ func ParsePacketMetadata(metadata string) (*PacketForwardMetadata, error) { | |||
| var routingInfo ModuleRoutingInfo | |||
| if raw.Autopilot.Stakeibc != nil { | |||
| // override the stride address with the receiver address | |||
| raw.Autopilot.Stakeibc.StrideAddress = raw.Autopilot.Receiver | |||
| moduleCount++ | |||
| routingInfo = *raw.Autopilot.Stakeibc | |||
| } | |||
| if raw.Autopilot.Claim != nil { | |||
| // override the stride address with the receiver address | |||
| if m.Action != "LiquidStake" { | ||
| return errorsmod.Wrapf(ErrUnsupportedStakeibcAction, "action %s is not supported", m.Action) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // Validate claim packet metadata includes the stride address | ||
| // Should we remove this struct? |
There was a problem hiding this comment.
yeah we should remove it, but I think you'll have to update the parsing logic cause it will no longer go down this path (although it's possible it still goes down it if you pass "claim": {}
There was a problem hiding this comment.
Circling back on this - I don't think it's super trivial to remove it.
In the main IBC callback, we have a generic ModuleRoutingInfo struct that we need to switch on to decide whether it's a stakeibc or claim packet.
--
I think we have two options:
- (1) Keep
ClaimPacketMetadata, but make a change to the switch statement - (2) Change the autopilot schema to have
actionat the root level instead of the module name
{"autopilot": {"action": "LiquidStake", ...}}
// instead of
{"autopilot": {"stakeibc": {"action": "LiquidStake"}}}
My preference is (2), but it might break integrations if anyone's currently using autopilot
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
This approach seems cleaner to me as well |
|
@asalzmann I know I approved this already, but let's revisit this after the other PRs are merged. It's possible they'll influence the schema |
|
Since this isn't critical to merge, will leave this until all other PRs are reviewed |
|
closing this for the time being, we can revist the schema later |
Context and purpose of the change
Removes the unused
StrideAddress(this was just being set to receiver).Brief Changelog
data->fungibleTokenPacketDataStrideAddressinx/autopilot/types/parser.gofungibleTokenPacketData.ReceiverwherepacketMetadata.StrideAddresswas previously used in both airdrop address linking and autopilot liquid stakingTODO