feat(core-api): hasTransactionFinality() on connector API #812
Conversation
jscode017
requested review from
jonathan-m-hamilton and
petermetz
as code owners
|
@jscode017 That test failure is shown by the CI logs is not related to the changes you made, if you rebase your branch onto #810 (and mark it as a dependency of this PR) then the tests should start passing. Alternatively just wait a little until #810 gets merged and then rebase onto |
...ages/cactus-plugin-ledger-connector-besu/src/main/typescript/plugin-ledger-connector-besu.ts
Outdated
Show resolved
Hide resolved
...es/cactus-plugin-ledger-connector-corda/src/main/typescript/plugin-ledger-connector-corda.ts
Outdated
Show resolved
Hide resolved
|
@jscode017 Rebasing onto |
| } | ||
| return isInConsensusAlgorithmFamiliesWithTxFinality; | ||
| } | ||
| //export defualt consensusHasTransactionFinality; |
There was a problem hiding this comment.
@jscode017 Recommend to remove commented lines unless you really need them for some specific reason (assuming here this was just forgotten and no longer needed at all)
There was a problem hiding this comment.
My bad, forget to do a little clean up
| @@ -4,3 +4,4 @@ export { | |||
| ConsortiumRepository, | |||
| IConsortiumRepositoryOptions, | |||
| } from "./consortium-repository"; | |||
| export * from "./consensus-has-transaction-finality"; | |||
There was a problem hiding this comment.
@jscode017 Recommendation here is to explicitly export not with the * character (easier to read what exactly goes in the public API that way because then there's no need to open the file up itself and skim through it's exports).
There's exceptions to this practice when there's a lot of exports for example in a generated file, but here it's just the one so I'd say it's easier on the reader if you export it explicitly
export { theFunction } from "./consensus-has-transaction-finality";
| @@ -128,7 +131,11 @@ export class PluginLedgerConnectorStub | |||
| > { | |||
| return ConsensusAlgorithmFamily.AUTHORITY; | |||
| } | |||
| public async hasTransactionFinality(): Promise<boolean> { | |||
There was a problem hiding this comment.
I just copy paste the implementation in those *-plugin-ledger-connectors. But I am not so sure, since it is just a stub, so maybe you would want just return false or do some other things?
There was a problem hiding this comment.
It's perfectly fine as it is IMO. The stub is just there to help with testing so the expectation is that if someone wants to use it to simulate different consensus algorithm families then they can either sub-class it or override the default behavior some other way.
The only thing I need now is the commits to be squashed into a single one so that there are no in-between states (every commit on the main branch should be valid, passing, tests, etc.
So if you squash and then force push (make sure to have the commit signed off) then I'll smash that approve button right after that. Thank you for the contribution and working with me on hammering out the details!
There was a problem hiding this comment.
Thanks for the help for my first pull request!
There was a problem hiding this comment.
@jscode017 About to approve and merge this, but I wanted to give one more piece of advice regarding the mechanics of how PR reviews are done:
If someone reviews the PR (e.g. when I literally click button submit review) from that point on it won't notify me until you hit the 're-request review' button (that little refresh icon next to my name in the top right corner among the reviewers).
This is not an actual issue, but I wanted to explain this anyway because I just saw by chance that you had fixed everything that I was asking for more than a week ago but I only noticed it now (otherwise I would've approved+merged the same day)
There was a problem hiding this comment.
Sorry for that, I would hit the 're-request review' next time.
There was a problem hiding this comment.
No worries at all, thank you again for the PR!
…-cacti#354 Primary change(s): ------------------ 1. Add a feature of asking the connector at runtime whether its ledger has transaction finality guaranteed or not Fixes: hyperledger-cacti#354 Signed-off-by: jscode017 <jasonhack518@gmail.com>
Adding function hasTransactionFinality() on connector API, to let ledger query whether the configured consensus has transaction finality at run time.
Fixes #354