Murisi/sdk refactor rebased #1963

murisi · 2023-10-03T08:01:12Z

Describe your changes

Created an SDK interface in order to streamline common operations like building, signing, and submitting constructions. Also modified the codebase to use this new interface as a means of testing its usability. More specifically, the following changes have been made:

Provided standard implementations of "WalletUtils" (WalletIo + WalletStorage) and ShieldedUtils so that users do not have to implement them themselves
Alleviated the user from having to specify a client, shielded context, wallet context, and input/output stream for each transaction by bundling these into one object
Now supply default field values in the builders for each of the transaction types in order to reduce the verbosity of making transactions
Integrated the saving and loading of wallets into the "WalletUtils" trait so that these operations can be done without knowing the underlying wallet storage
Combined the construction of a transaction and its corresponding singing data in order to reduce the number of calls an SDK user to make from transaction construction through to submission
- Doing this also reduced the number of parameters SDK users have to supply to transaction building functions because they now internally compute the signing data
Made the SDK interface work for parallel use cases by using locks for the wallet and the shielded context. Formerly, the user had to inefficiently construct new wallets and shielded contexts from file storage for each thread.
Separated SDK functionality (everything required to construct, sign, and send transactions; perform queries) out of the shared crate and into a new crate named sdk. Hence the dependency graph is now more like shared -> sdk -> core.

See an example of this PR's usage here: anoma/namada-sdk-starter#1 .

Indicate on which release or other PRs this topic is based on

v0.23.0

Checklist before merging to `draft`

I have added a changelog
Git history is in acceptable state

…ces.

sug0

these changes seem generally good to me. the only thing(s) I'm worried about are the conflicts with the chain of PRs whose tip is #1957 😅

apps/src/bin/namada-relayer/main.rs

batconjurer · 2023-10-09T09:08:23Z

shared/src/ledger/mod.rs

+
+#[async_trait::async_trait(?Send)]
+/// An interface for high-level interaction with the Namada SDK
+pub trait Namada<'a>: Sized {


nit: I'm not entirely happy with the name because of how it appears in the apps crate. It's amost always assigned to something called context. I feel like it should be NamadaCtx or NamadaApi or NamadaUtils or something.

The name context is a bit of a hangover from how we were naming instances of Context - this too is changeable. This trait is intended to be the primary interface for SDK users (who won't be aware of its apps crate usage and our naming distinctions), so I was thinking more about the best look/outcome in this context: https://github.com/anoma/namada-sdk-starter/blob/murisi/updating-sdk-usage/src/main.rs. Also, SDK users (and we) will probably be typing &impl Namada<'a> a lot, so I'm keen to minimize the name's length. That being said, I'm not too attached to any particular name, and we can always export/import as.

It's not that a big a deal. The pedant in me thinks, oh we aren't implemeting Namada with this but an application interface (my NamadaApp is an idea?), but in practice I don't think it will cause any confusion

batconjurer · 2023-10-09T09:10:08Z

shared/src/ledger/mod.rs

+}
+
+/// Provides convenience methods for common Namada interactions
+pub struct NamadaImpl<'a, C, U, V, I>


And I also would change this name too.

shared/src/ledger/mod.rs

batconjurer · 2023-10-09T09:19:49Z

shared/src/sdk/signing.rs

-            )
-            .await
-            .expect("Expected to be able to validate fee");
+            let validated_fee_amount =


I just remember, are we allowed to have panics in the sdk still?

any unwrap() or expect() calls which have a chance to fail should be replaced with results or options

Thanks. You're correct: panics are not allowed in the SDK. Since SDK integration is urgent - perhaps we can make a future PR where we identify the remaining occurrences of panics and fix them?

… added null IO implementation.

tzemanovic · 2023-10-11T08:16:13Z

shared/src/ledger/mod.rs

+    /// Used to send and receive messages from the ledger
+    pub client: &'a C,
+    /// Stores the addresses and keys required for ledger interactions
+    pub wallet: RwLock<&'a mut Wallet<U>>,


Unfortunately, I don't think the lock on the wallet makes it parallelizable as one task might overwrite the changes of another task because in-memory changes are not synced to fs without an explicit save() call. We have an issue to fix this (#50 - the wallet should lock internally during any mutable operations on it and only release the lock after implicitly saving to fs with as small scope as possible to avoid contention), but I'd like to avoid the illusion of this being thread-safe before that's done as it can lead to loss of keys. I'm not quite sure what to do with this until then

Agreed, this PR buys us asynchrony within a Namada application at the cost of safety for multiple running Namada applications. (We might use it in this parallel application for instance: https://github.com/anoma/namada-sdk-starter/blob/murisi/updating-sdk-usage/src/main.rs .) Since this PR does fix other SDK and API issues and the locking issue is not so trivial, could we perhaps merge this PR and then follow up with a fix for wallet safety?

One quick way to address the key loss issue may be to leave the API as-is and modify the save method of the wallet and shielded context to: 1) obtain lock on file 2) load the file into memory 3) merge the new changes into that file 4) save the merged file 5) release the lock. See

namada/shared/src/sdk/masp.rs

Line 633 in 3f979bf

/// Merge data from the given shielded context into the current shielded

. Then we will have more time to implement #50 while knowing that keys are not being lost.

That's a great idea! I think we should go with that

sug0

I had some PRs open that touched the sdk. I need your help fixing them later @murisi haha

… shielded context.

grarco

Only reviewed from 17c3e6c onward but looks good to me

* origin/murisi/sdk-refactor-rebased: Added a change log entry. Changes to enable better usage of the SDK in the absence of wallet or shielded context. Separated an SDK crate out of the shared crate. SDK can now query for the native token address from the network. Also added null IO implementation. Reintegrated generic IO support. Enabled parallel usage of the Namada trait using read and write locks. Removed unnecessary requirement for mutable references in Namada trait. Added function to construct DenominatedAmounts from Amounts. Made the ShieldedUtils trait similar to the WalletStorage trait. Adding saving and loading function to Wallet. Moved FsShieldedUtils into the SDK behind a feature flag. Allow the prototypical Tx builder to be modified in NamadaImpl instances. Created builders and constructors for each type of transaction. Combined construction of signing data with transaction construction.

murisi added 9 commits October 1, 2023 22:03

Combined construction of signing data with transaction construction.

fe6c3eb

Created builders and constructors for each type of transaction.

16425d5

Allow the prototypical Tx builder to be modified in NamadaImpl instan…

d10ba96

…ces.

Moved FsShieldedUtils into the SDK behind a feature flag.

e471b6c

Adding saving and loading function to Wallet.

1142e64

Made the ShieldedUtils trait similar to the WalletStorage trait.

f99eaa3

Added function to construct DenominatedAmounts from Amounts.

79f006a

Removed unnecessary requirement for mutable references in Namada trait.

c775ec7

Enabled parallel usage of the Namada trait using read and write locks.

9f06a4b

murisi force-pushed the murisi/sdk-refactor-rebased branch 7 times, most recently from 265c58b to 0572aa3 Compare October 6, 2023 13:10

Reintegrated generic IO support.

b1bc845

murisi force-pushed the murisi/sdk-refactor-rebased branch from 0572aa3 to b1bc845 Compare October 6, 2023 13:33

murisi marked this pull request as ready for review October 6, 2023 14:49

murisi requested review from tzemanovic, sug0 and batconjurer October 6, 2023 14:49

sug0 previously approved these changes Oct 9, 2023

View reviewed changes

batconjurer reviewed Oct 9, 2023

View reviewed changes

apps/src/bin/namada-relayer/main.rs Outdated Show resolved Hide resolved

batconjurer reviewed Oct 9, 2023

View reviewed changes

shared/src/ledger/mod.rs Outdated Show resolved Hide resolved

batconjurer reviewed Oct 9, 2023

View reviewed changes

murisi mentioned this pull request Oct 9, 2023

Murisi/updating sdk usage anoma/namada-sdk-starter#1

Merged

murisi dismissed sug0’s stale review via 5b148cb October 9, 2023 13:26

SDK can now query for the native token address from the network. Also…

615ebc8

… added null IO implementation.

murisi force-pushed the murisi/sdk-refactor-rebased branch from 5b148cb to 615ebc8 Compare October 9, 2023 13:44

tzemanovic reviewed Oct 11, 2023

View reviewed changes

Separated an SDK crate out of the shared crate.

17c3e6c

murisi requested review from sug0 and grarco October 12, 2023 06:55

sug0 previously approved these changes Oct 12, 2023

View reviewed changes

murisi added 2 commits October 12, 2023 09:55

Changes to enable better usage of the SDK in the absence of wallet or…

f892ab4

… shielded context.

Added a change log entry.

94b2d4b

murisi dismissed sug0’s stale review via 94b2d4b October 12, 2023 08:06

murisi added a commit that referenced this pull request Oct 12, 2023

Merge branch 'murisi/sdk-refactor-rebased' (#1963)

3cd3970

murisi mentioned this pull request Oct 12, 2023

Draft #1980

Closed

murisi added SDK merged to draft labels Oct 12, 2023

sug0 mentioned this pull request Oct 12, 2023

Move Ethereum bridge SDK functionality #1941

Closed

2 tasks

grarco approved these changes Oct 12, 2023

View reviewed changes

sug0 mentioned this pull request Oct 13, 2023

Phase out Halt abstractions #1953

Merged

2 tasks

murisi mentioned this pull request Oct 23, 2023

Murisi/sdk re exports #2033

Merged

2 tasks

tzemanovic mentioned this pull request Oct 24, 2023

0.24.0 rc2 #2039

Merged

murisi merged commit 94b2d4b into main Oct 24, 2023
12 of 14 checks passed

murisi deleted the murisi/sdk-refactor-rebased branch October 24, 2023 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Murisi/sdk refactor rebased #1963

Murisi/sdk refactor rebased #1963

murisi commented Oct 3, 2023 •

edited

Loading

sug0 left a comment

batconjurer Oct 9, 2023

murisi Oct 9, 2023

batconjurer Oct 9, 2023

batconjurer Oct 9, 2023

batconjurer Oct 9, 2023

sug0 Oct 9, 2023

murisi Oct 9, 2023

tzemanovic Oct 11, 2023

murisi Oct 11, 2023 •

edited

Loading

tzemanovic Oct 11, 2023

sug0 left a comment

grarco left a comment

Murisi/sdk refactor rebased #1963

Murisi/sdk refactor rebased #1963

Conversation

murisi commented Oct 3, 2023 • edited Loading

Describe your changes

Indicate on which release or other PRs this topic is based on

Checklist before merging to draft

sug0 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

murisi Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sug0 left a comment

Choose a reason for hiding this comment

grarco left a comment

Choose a reason for hiding this comment

murisi commented Oct 3, 2023 •

edited

Loading

Checklist before merging to `draft`

murisi Oct 11, 2023 •

edited

Loading