feat: local wallets #3976

lemmih · 2024-02-20T12:41:29Z

Summary of changes

Changes introduced in this pull request:

Add --remote-wallet flag to forest-wallet. Without it, the tool will use a locally stored wallet.
The Forest node still has a separate wallet for compliance with Lotus.

Reference issue to close (if applicable)

Closes #3957

Change checklist

I have performed a self-review of my own code,
I have made corresponding changes to the documentation,
I have added tests that prove my fix is effective or that my feature works (if possible),
I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

LesnyRumcajs

It'd be great to have the recommended usage (and alternative one) documented somewhere. Wallet handling is something that should be straightforward and as simple as possible for the end user.
Is the --remote-wallet path covered by tests?

LesnyRumcajs · 2024-03-07T11:31:58Z

src/rpc_api/mod.rs

@@ -58,7 +58,7 @@ pub static ACCESS_MAP: Lazy<HashMap<&str, Access>> = Lazy::new(|| {
    // Message Pool API
    access.insert(mpool_api::MPOOL_GET_NONCE, Access::Read);
    access.insert(mpool_api::MPOOL_PENDING, Access::Read);
-    access.insert(mpool_api::MPOOL_PUSH, Access::Write);
+    access.insert(mpool_api::MPOOL_PUSH, Access::Read);


Anyone can push to mpool? Is it safe?

I'm somewhat convinced that it is safe. Lotus has three ways of receiving signed messages: MPoolPush, MPoolPushUntrusted, and p2p pubsub. MPoolPushUntrusted has a few more sanity checks and a lower cap on outstanding messages. But any peer can send messages through p2p pubsub which does not have a limit and acts just like MPoolPush. Any security we add to MPoolPush is merely security through obscurity since we (and Lotus) blindly accept messages through pubsub.

Makes sense, thanks for clarification.

Oh, and glif.io doesn't even expose MPoolPushUntrusted. But they do let everyone use MPoolPush. It's kinda strange. All in all, I think we're best off making MPoolPush public and then making it as difficult to spam as possible.

Once we have rate-limiting implemented, we could by default limit it to zero.

Added a note in the code for why we differ from Lotus.

LesnyRumcajs · 2024-03-07T11:33:27Z

src/wallet/subcommands/wallet_cmd.rs

+            let Some(dir) = ProjectDirs::from("com", "ChainSafe", "Forest-Wallet") else {
+                bail!("Failed to find wallet directory");
+            };
+            // FIXME: Support encrypted wallets


Do we have a tracking issue for this?

Ah, no, I wanted to implement it in this PR, but I forgot. I'll make sure to add it.

LesnyRumcajs · 2024-03-07T11:50:54Z

src/wallet/subcommands/wallet_cmd.rs

+                let StrictAddress(address) = StrictAddress::from_str(&address)
+                    .with_context(|| format!("Invalid address: {address}"))?;
+
+                if let Some(mut keystore) = local_keystore {


This if-else proliferation makes it difficult to reason about the code. Can we have it abstracted away?

could probably be converted into map_or for all the occurrences or any other more eloquent solution.

Another option is to get rid of else and use return in the if block.

Cleaned it up a bit but it's still kinda ugly. Suggestions welcome.

LesnyRumcajs · 2024-04-05T15:50:51Z

@lemmih is it still WIP (see title)?

lemmih · 2024-04-08T08:03:38Z

@lemmih is it still WIP (see title)?

Fixed.

LesnyRumcajs

Should forest-cli send be deprecated / removed?

documentation/src/wallet.md

LesnyRumcajs · 2024-04-08T08:51:44Z

documentation/src/wallet.md

It might make sense to paste this to Grammarly to fix potential typos

Yeah. I was too fast and too furious.

A side note, don't we have spell checks in CI for md files?

I could have sworn we had it at one point.

LesnyRumcajs · 2024-04-08T09:03:11Z

src/wallet/subcommands/wallet_cmd.rs

+                    gas_limit: gas_limit as u64,
+                    gas_fee_cap: gas_feecap,
+                    gas_premium,
+                    // JANK(aatifsyed): Why are we using a testing build of fvm_shared?


What do we do with this?

It's a wrinkle in fvm_shared. I guess we could send a patch upstream.

Co-authored-by: Hubert <hubert@chainsafe.io>

lemmih · 2024-04-08T09:26:36Z

Should forest-cli send be deprecated / removed?

Yeah, it should be deprecated. Done.

hanabi1224 · 2024-04-08T10:10:39Z

CHANGELOG.md

@@ -113,6 +113,12 @@ Mandatory release that includes:
 - [#3955](https://github.com/ChainSafe/forest/pull/3955) Added support for the
  NV22 _Dragon_ network upgrade, together with the required state migration.

+### Changed
+
+- [#3976](https://github.com/ChainSafe/forest/pull/3976) `forest-wallet`


If this is merged after #4152 I believe the position of this log entry needs to be manually adjusted

lemmih · 2024-04-08T13:26:42Z

@LesnyRumcajs @hanabi1224 Need re-review due to merge mishaps.

lemmih added 11 commits February 20, 2024 13:40

basic support for local wallets

d210bf6

move send command into forest-wallet

0b5221c

apply clippy suggestion

8167ada

copy base implementation

4ef4495

Merge branch 'main' into lemmih/local-wallet

f0e92c3

Attempt to sign transfer messages outside of Forest

3f004e4

Merge branch 'main' into lemmih/local-wallet

1653679

remove debug statements

859d565

Pushing a signed message should be open to everyone

6126a39

add missing file

bbb458c

Merge branch 'main' into lemmih/local-wallet

45230ac

lemmih changed the title ~~[WIP] feat: local wallets~~ feat: local wallets Mar 6, 2024

Add changelog entry

ef83abf

lemmih marked this pull request as ready for review March 6, 2024 12:04

lemmih requested a review from a team as a code owner March 6, 2024 12:04

lemmih requested review from ruseinov and LesnyRumcajs and removed request for a team March 6, 2024 12:04

lemmih marked this pull request as draft March 6, 2024 12:05

lemmih marked this pull request as ready for review March 6, 2024 12:29

LesnyRumcajs reviewed Mar 7, 2024

View reviewed changes

lemmih marked this pull request as draft March 8, 2024 10:54

lemmih changed the title ~~feat: local wallets~~ [WIP] feat: local wallets Mar 18, 2024

lemmih added 7 commits April 3, 2024 14:00

Merge branch 'main' into lemmih/local-wallet

c4059fb

fix merge mishaps

02b4967

fix md formatting

135e54f

refactor local/remote wallet handling

919caab

documentation

93bc491

support encrypted wallets

3837fb6

apply clippy suggestions

abaf2af

lemmih added 4 commits April 4, 2024 14:08

Test both local and remote wallet handling

6aa71d8

Merge branch 'main' into lemmih/local-wallet

44d9702

fix typo

bc8a5b7

fix yet another typo

29494fc

lemmih marked this pull request as ready for review April 4, 2024 14:03

lemmih added 3 commits April 5, 2024 14:51

update documentation for forest-wallet

c2b3f15

Merge branch 'main' into lemmih/local-wallet

7a75c67

fix merge mishap

f89b5f9

lemmih changed the title ~~[WIP] feat: local wallets~~ feat: local wallets Apr 6, 2024

LesnyRumcajs approved these changes Apr 8, 2024

View reviewed changes

lemmih and others added 4 commits April 8, 2024 11:07

Fix typo

f74a5f9

Co-authored-by: Hubert <hubert@chainsafe.io>

Fix another typo

2f5ea96

Co-authored-by: Hubert <hubert@chainsafe.io>

re-write documentation for clarity

6fc7530

deprecate the forest-cli send command

02956cc

hanabi1224 approved these changes Apr 8, 2024

View reviewed changes

hanabi1224 reviewed Apr 8, 2024

View reviewed changes

lemmih added 2 commits April 8, 2024 15:03

Merge branch 'main' into lemmih/local-wallet

44ad119

clean-up

e3e31f4

lemmih enabled auto-merge April 8, 2024 13:26

ruseinov approved these changes Apr 8, 2024

View reviewed changes

lemmih added this pull request to the merge queue Apr 8, 2024

Merged via the queue into main with commit 070202b Apr 8, 2024
27 checks passed

lemmih deleted the lemmih/local-wallet branch April 8, 2024 14:11

lemmih mentioned this pull request Apr 11, 2024

Remove Keystore #1517

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: local wallets #3976

feat: local wallets #3976

lemmih commented Feb 20, 2024 •

edited

LesnyRumcajs left a comment

LesnyRumcajs Mar 7, 2024

lemmih Mar 7, 2024

LesnyRumcajs Mar 7, 2024

lemmih Mar 7, 2024

LesnyRumcajs Mar 7, 2024

lemmih Apr 4, 2024

LesnyRumcajs Mar 7, 2024

lemmih Mar 7, 2024

lemmih Apr 4, 2024

LesnyRumcajs Mar 7, 2024

ruseinov Mar 11, 2024

lemmih Apr 4, 2024

LesnyRumcajs commented Apr 5, 2024

lemmih commented Apr 8, 2024

LesnyRumcajs left a comment

LesnyRumcajs Apr 8, 2024

lemmih Apr 8, 2024

hanabi1224 Apr 8, 2024

lemmih Apr 8, 2024

LesnyRumcajs Apr 8, 2024

lemmih Apr 8, 2024

lemmih commented Apr 8, 2024

hanabi1224 Apr 8, 2024

lemmih commented Apr 8, 2024

feat: local wallets #3976

feat: local wallets #3976

Conversation

lemmih commented Feb 20, 2024 • edited

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

LesnyRumcajs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LesnyRumcajs commented Apr 5, 2024

lemmih commented Apr 8, 2024

LesnyRumcajs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemmih commented Apr 8, 2024

Choose a reason for hiding this comment

lemmih commented Apr 8, 2024

lemmih commented Feb 20, 2024 •

edited