-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
DRY kv/set #2319
DRY kv/set #2319
Conversation
group-income Run #3152
Run Properties:
|
Project |
group-income
|
Branch Review |
feature/2289-dry-kv-set
|
Run status |
Passed #3152
|
Run duration | 09m 09s |
Commit |
4431b574d9 ℹ️: Merge 13c390ff6c12bab614f4d4fddf83b72993d77632 into c93398fe79ee6bdf2b9b66def7bc...
|
Committer | Ricardo Iván Vieitez Parra |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
10
|
Skipped |
0
|
Passing |
111
|
View all changes introduced in this branch ↗︎ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @corrideat!
One slight change request:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor organizational and comment-related change request
'chelonia/kv/queuedSet': ({ contractID, key, data, onconflict, encryptionKeyName = 'cek', signingKeyName = 'csk' }) => { | ||
return sbp('chelonia/queueInvocation', contractID, () => { | ||
return sbp('chelonia/kv/set', contractID, key, data, { | ||
encryptionKeyId: sbp('chelonia/contract/currentKeyIdByName', contractID, encryptionKeyName), | ||
signingKeyId: sbp('chelonia/contract/currentKeyIdByName', contractID, signingKeyName), | ||
onconflict | ||
}) | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request:
- could be this placed next to the the existing selector
'chelonia/kv/set'
so that it's easier to see? - could you please add a comment above it explaining why usually calling this version is preferred and what this queueing accomplishes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be this placed next to the the existing selector 'chelonia/kv/set' so that it's easier to see?
I think chelonia.js
is already cluttered as it is. The reason for putting this in a different file is also that IMHO this belongs in a possibly optional library as it makes some assumptions about which keys are defined (i.e., the csk and the cek) which isn't done in Chelonia currently.
When encryptedAction is moved into Chelonia, it would also be placed in this new file, because it also is a 'suggested' way of doing a common action, but not something that applies to all situations.
Could you please add a comment above it explaining why usually calling this version is preferred
Sure, although calling this version isn't 'preferred', just less much verbose in a common situation. I can explain that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, if you want to leave it in that file, then please add a comment above 'chelonia/kv/set'
to tell people to look at 'chelonia/kv/queuedSet'
which is preferable (and where to find that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful!
Closes #2289