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

Auto-provision smart wallet #8574

Closed
wants to merge 19 commits into from

Conversation

mhofman
Copy link
Member

@mhofman mhofman commented Nov 29, 2023

closes: #8558

Description

This change implements smart wallet auto-provisioning by:

  • Checking if a smart wallet is already provisioned in vstorage during admission checks
  • Charging a new beans based fee if not
  • Generating a provision action when processing the smart wallet message based on whether the smart wallet is provisioned already or not

It does not mark whether a smart wallet provision is pending, and as such may result in provision beans fees being charged multiple times if smart wallet messages are submitted and included in new blocks before the provision message is executed by swingset.

This PR includes some necessary changes to the upgrade handler:

  • Generalizing the swingset parameters logic to always add new parameters using their default value in the upgrade handler

Security Considerations

The provision step in swingset does not change and still provides a nominal amount of IST during provisioning, for each provision message handled. As such the beans amount charged for auto-provisioning a wallet should be higher than the nominal amount transferred from the provision pool.

Scaling Considerations

None

Documentation Considerations

TBD

Testing Considerations

TBD

Upgrade Considerations

Requires a new chain software consensus upgrade.
Added required logic to the upgrade handler for the new fee.

@JimLarson
Copy link
Contributor

High-level comments:

Impressive plumbing to get that all wired through!

I'm a little concerned about the additional complexity for trying to suppress duplicate auto-provisioning within the same transaction. We're already resigned to having duplicate auto-provision requests in different blocks while the first request is still queued, so maybe that's an acceptable way to handle the now-impossible case of multiple wallet messages in a single transaction? It would be great if we could drop the customization of sdk.Context. Doing so would require both the ante handler and the msg server to check vstorage for the existence of the wallet, but I don't think that's prohibitive.

@mhofman
Copy link
Member Author

mhofman commented Nov 30, 2023

It would be great if we could drop the customization of sdk.Context. Doing so would require both the ante handler and the msg server to check vstorage for the existence of the wallet, but I don't think that's prohibitive.

It felt wrong for the msg server to assume all unprovisioned smart wallet messages allowed by the ante handler were eligible for auto provisioning, seeing how decoupled they are, but I suppose that's a fair assumption to make. I do agree it would simplify the logic, and maybe with sufficient comments in both directions it'd be acceptable.

@mhofman
Copy link
Member Author

mhofman commented Nov 30, 2023

@JimLarson

I removed the ctx logic. In hindsight, it's much better because it wouldn't be needed anyway if/when we implement marking of pending provisions.

We could potentially re-use the egress marking of explicit provision to implement this marking now.

@mhofman mhofman linked an issue Nov 30, 2023 that may be closed by this pull request
@mhofman mhofman force-pushed the mhofman/8558-auto-provision-mainnet1B branch from cf6f4d2 to cdbaa77 Compare December 1, 2023 19:33
@mhofman
Copy link
Member Author

mhofman commented Dec 1, 2023

Addressed feedback, then fixed-up: cf6f4d2...cdbaa77

@mhofman mhofman changed the base branch from release-mainnet1B to dev-upgrade-13 December 1, 2023 19:35
@mhofman mhofman force-pushed the mhofman/8558-auto-provision-mainnet1B branch from cdbaa77 to 6b5dcae Compare December 1, 2023 19:36
@mhofman mhofman added the force:integration Force integration tests to run on PR label Dec 1, 2023
Copy link
Contributor

@JimLarson JimLarson left a comment

Choose a reason for hiding this comment

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

Much simpler now! But a few remaining cleanups. Should be quick.

golang/cosmos/app/app.go Show resolved Hide resolved
golang/cosmos/x/swingset/keeper/keeper.go Show resolved Hide resolved
golang/cosmos/x/swingset/types/msgs.go Outdated Show resolved Hide resolved
golang/cosmos/x/swingset/types/msgs.go Outdated Show resolved Hide resolved
golang/cosmos/x/swingset/keeper/keeper.go Outdated Show resolved Hide resolved
golang/cosmos/x/swingset/types/params.go Outdated Show resolved Hide resolved
golang/cosmos/vm/controller.go Outdated Show resolved Hide resolved
@mhofman
Copy link
Member Author

mhofman commented Dec 6, 2023

Rolled into #8623

@mhofman mhofman closed this Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Charge provisioning fee if smart wallet absent
3 participants