Skip to content
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

Refresh autopilot LS + forward #996

Merged
merged 12 commits into from
Jan 8, 2024
Merged

Refresh autopilot LS + forward #996

merged 12 commits into from
Jan 8, 2024

Conversation

asalzmann
Copy link
Contributor

@asalzmann asalzmann commented Nov 28, 2023

Closes: #771

Context and purpose of the change

Add autopilot liquid stake + forward support. This allows autopilot liquid stakers to specify a receiving address and IBC channel, where stTokens will be forwarded after being liquid staked.

Open questions

  • What should the IBC transfer timeout be?
  • Should we use a hash for the sender address (similar to PFM) to avoid the security vulnerability discovered by SCV?

Next steps

  • Remove StrideAddress (it's unused)
  • Implement redeem functionality

Brief Changelog

  • Add integration test
  • Update autopilot keeper to have access to the transfer keeper
  • Add IbcReceiver and TransferChannel to StakeibcPacketMetadata
  • Add IBCTransferStAsset function, called at the end of RunLiquidStake

Test plan

  • Add integration test
    • TODO: add additional test for missing IbcReceiver and TransferChannel fields
  • Manually test LS + forward to a different network

@asalzmann asalzmann requested a review from a team December 5, 2023 02:38
@sampocs sampocs added the v17 label Dec 6, 2023
Copy link
Collaborator

@sampocs sampocs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very clean! looks good so far, pending the sender change!

x/autopilot/keeper/keeper.go Outdated Show resolved Hide resolved
dockernet/tests/integration_tests.bats Show resolved Hide resolved
x/autopilot/types/parser.go Outdated Show resolved Hide resolved
x/autopilot/keeper/liquidstake.go Outdated Show resolved Hide resolved
x/autopilot/keeper/liquidstake.go Outdated Show resolved Hide resolved
x/autopilot/keeper/liquidstake.go Outdated Show resolved Hide resolved
x/autopilot/keeper/liquidstake.go Show resolved Hide resolved
@riley-stride riley-stride self-requested a review December 14, 2023 21:24
x/autopilot/keeper/liquidstake.go Show resolved Hide resolved
x/autopilot/keeper/liquidstake.go Outdated Show resolved Hide resolved
x/autopilot/module_ibc.go Outdated Show resolved Hide resolved
x/autopilot/types/parser.go Outdated Show resolved Hide resolved
dockernet/tests/integration_tests.bats Show resolved Hide resolved
dockernet/tests/integration_tests.bats Show resolved Hide resolved
x/autopilot/keeper/liquidstake.go Show resolved Hide resolved
Copy link
Contributor

@shellvish shellvish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me, thanks for doing this! Fwiw, I think this is also working on testnet now, but we can also add more connections there and test edge cases (e.g. sending to a new network)

x/autopilot/keeper/liquidstake.go Show resolved Hide resolved
x/autopilot/keeper/liquidstake.go Show resolved Hide resolved
x/autopilot/keeper/liquidstake.go Outdated Show resolved Hide resolved
x/autopilot/keeper/liquidstake.go Outdated Show resolved Hide resolved
@sampocs
Copy link
Collaborator

sampocs commented Jan 3, 2024

@asalzmann fyi, unit tests for this are in #1041

@asalzmann
Copy link
Contributor Author

Next steps for this PR

  • Merge in other PRs that depend on this PR
  • Do manual test in description
  • (maybe - unsure if needed) add another integration test

@sampocs sampocs force-pushed the autopilot-ls-and-forward branch from 3b8680f to e87bdd5 Compare January 8, 2024 20:18
@sampocs sampocs merged commit 138c325 into main Jan 8, 2024
10 checks passed
@sampocs
Copy link
Collaborator

sampocs commented Jan 8, 2024

(for the record)...

Merge in other PRs that depend on this PR

We decided to merge this first

Do manual test in description

Done and resolved comment

(maybe - unsure if needed) add another integration test

We decided it wasn't necessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants