Refactor basewallet and corresponding consumers #843
Conversation
Codecov Report
@@ Coverage Diff @@
## main #843 +/- ##
==========================================
- Coverage 48.96% 45.16% -3.81%
==========================================
Files 429 429
Lines 34225 34238 +13
Branches 7594 7590 -4
==========================================
- Hits 16759 15464 -1295
- Misses 12208 13848 +1640
+ Partials 5258 4926 -332
Flags with carried forward coverage won't be shown. Click here to find out more.
|
7b5b067 to
7982800
Compare
|
hi @tech-bash note the failing jobs - take a look how we run those in CI and try to reproduce locally |
|
Yes sir, on it already! |
c4fd1a7 to
baa011e
Compare
There was a problem hiding this comment.
Hey, nice work so far!
I noticed in some places you've changed the parameters of BaseWallet methods to take String instead of &str. For consistency, could you please change them back?
E.g.
you've changed:
async fn update_wallet_record_value(&self, xtype: &str, id: &str, value: &str)
to:
async fn update_wallet_record_value(&self, xtype: &str, id: &str, value: String)
but value should stay as &str.
This happens in a few other places too, just have a look thru the diff on this PR. thanks!
c3585d6 to
cf856b9
Compare
Signed-off-by: Bhavy Airi <airiragahv@gmail.com>
e505514 to
31e0346
Compare
| async fn add_wallet_record_tags(&self, xtype: &str, id: &str, tags_json: &str) -> VcxCoreResult<()> { | ||
| indy::wallet::add_wallet_record_tags(self.wallet_handle, xtype, id, tags_json).await | ||
| async fn add_wallet_record_tags(&self, xtype: &str, id: &str, tags: HashMap<String, String>) -> VcxCoreResult<()> { | ||
| let tags_json = serde_json::to_string(&tags)?; |
There was a problem hiding this comment.
We serialize hashamp to string, just to deserialize it inside subsequent add_wallet_record_tags into Hashmap again. Can we avoid this?
There was a problem hiding this comment.
We are taking the typed parameters and then converting them back to JSON before passing them to indy::wallet::abc.
There was a problem hiding this comment.
are you intending to say that we should only have hashmap as the parameter type and not &str?
|
|
||
| async fn get_wallet_record(&self, xtype: &str, id: &str, options_json: &str) -> VcxCoreResult<String>; | ||
| async fn get_wallet_record(&self, xtype: &str, id: &str, options: &str) -> VcxCoreResult<String>; |
There was a problem hiding this comment.
The options could actually be well typed, but its type depends on what kind of options the underlying implementation accepts.
This is slightly more advanced, but we could use associated types here. You would defined something like
pub trait BaseWallet: std::fmt::Debug + Send + Sync {
type GetRecordOptions;
type AddRecordOptions;
...
...
}
Then in particular implementation of the trait, for example in impl IndySdkWallet , you would inject the actual types. And of course, you would have to create those types (you can find out what the option objects should look like if you dig into libvdrtools code)
So not a hard requirement, but if you could do this it would be really great!
|
I am merging as is due inactivity. If you wold like to further continue improving this area, feel free to do so in a new PR. |
fix: #814
BaseWalletin the code base updated to use the new types rather than the JSON strings