feat: enable cohort to pass on_commit messages for messenger #94
Conversation
packages/cohort_banking/src/app.rs
Outdated
| "publish": { | ||
| "kafka": [ | ||
| { | ||
| "topic": "test.transfer.feedback", |
There was a problem hiding this comment.
Hardcoded topic as it's just the example app, and to minimise refactor.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #94 +/- ##
==========================================
- Coverage 57.35% 57.00% -0.36%
==========================================
Files 113 113
Lines 5666 5707 +41
==========================================
+ Hits 3250 3253 +3
- Misses 2416 2454 +38 ☔ View full report in Codecov by Sentry. |
| } | ||
| } | ||
|
|
||
| // TODO: GK - Add on commit action |
There was a problem hiding this comment.
Is this supported? If we support action why dont we add them to interface too?
There was a problem hiding this comment.
This is a good point, messenger is written in a way that it can accept any valid json, but will only process the supported types.
But on the other end, it would be nice if the sender is made aware on what they can send. Let me think on it and see how I can add this in at the cohort end. Thank for pointing this out!
| @@ -6,7 +6,8 @@ pub struct CandidateData { | |||
| pub readset: Vec<String>, | |||
| pub writeset: Vec<String>, | |||
| pub statemap: Option<Vec<HashMap<String, Value>>>, | |||
| // The "snapshot" is intentionally messing here. We will compute it ourselves before feeding this data to Talos | |||
| // The "snapshot" is intentionally missing here. We will compute it ourselves before feeding this data to Talos | |||
| pub on_commit: Option<Value>, | |||
There was a problem hiding this comment.
Please add this to readme as well;
add stricter type restrictions on the SDK end for messenger payload
| // #[derive(Debug, Serialize, Deserialize, Clone, Eq, PartialEq)] | ||
| // #[serde(rename_all = "camelCase")] | ||
| #[napi(object)] | ||
| pub struct JSKafkaAction { |
There was a problem hiding this comment.
Minor, not a blocker for PR merge. Just most of the data structures use "Js" prefix (here we seems to use "JS")
There was a problem hiding this comment.
Oops, that should be Js I will update that soon. Thanks for spotting it
cohort_sdk_client/README.md
Outdated
| @@ -113,10 +138,11 @@ export interface JsCertificationCandidate { | |||
| Before SDK can issue a certification request to Talos Certifier it needs some details from you. You will have to query your local database to fetch the following: | |||
| 1. Identifiers and version numbers of all objects involved in your transaction. These are known as `readset`, `writeset` and `readvers`. | |||
| 2. The copy of your transaction as one serializable object. It makes sense to describe your transaction as JSON object and serialise it to string. This is known as `statemap`. | |||
| 3. Any additional message to be published for candidate requests with committed decision outcome, can added to `onCommit` field. Currently the SDK supports only publishing to **Kafka**. | |||
There was a problem hiding this comment.
Did we want to say "can be added to" ?
onCommit, will need to publish new package. Probably safe to still push as minor as no one really consumes the JS packages yet.onCommitis hardcoded in the example apps, to keep it simple for example sake, thereby minimising refactor.