Skip to content
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

Adding new v2-byzcoin requests #2331

Merged
merged 2 commits into from
Oct 22, 2020
Merged

Adding new v2-byzcoin requests #2331

merged 2 commits into from
Oct 22, 2020

Conversation

ineiti
Copy link
Member

@ineiti ineiti commented Jul 14, 2020

The previous requests and instances were difficult to use for multi-instruction transactions.
Also, they didn't use the new Observable interface given by ByzCoin.

This PR adds a v2/ directory with some soon-to-be stable version of Darc and Coin instances.
Every instance can now be created as a BehaviorSubject that updates automatically whenever
a new block is available.
This allows to program a much more reactive programming of the user-interface: instead of
polling for new values, the UI can subscribe to the BehaviorSubjects and be updated
whenever something changes.

In the long run, other contracts should also find their way into byzcoin/v2.

Depends on #2392

@ineiti ineiti self-assigned this Jul 14, 2020
@ineiti ineiti added this to WIP in Cothority via automation Jul 14, 2020
@ineiti ineiti requested review from cgrigis and tharvik July 14, 2020 15:17
tharvik
tharvik previously approved these changes Jul 14, 2020
Copy link
Contributor

@tharvik tharvik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • does it make sense to put these in byzcoin/v2?

do you want to keep supporting the "v1" version? otherwise, you can merge the two

  • what about the differentiation between Darc, DarcBS, DarcInst? Same for Coin*?

maybe a namespace would make it more readable:

  • Darc.Contract which allow for Darc creation
  • Darc.BehaviorSubject which listen to new values
  • Darc.Instance which are elements generated by the BehaviorSubject, and each can generate new transactions
  • does it make sense to have the contraact-commands twice - once as general commands and once instance-specific?

not really, to quote the Zen of Python "There should be one-- and preferably only one --obvious way to do it."

Also please have a look at how multiple instructions are put into a single transaction in coinbs.spec.ts::"transfer coins".

one would need a monad language to make it even nicer :)

external/js/cothority/tsconfig.spec.json Outdated Show resolved Hide resolved
external/js/cothority/tsconfig.spec.json Outdated Show resolved Hide resolved
external/js/cothority/src/byzcoin/v2/darcsBS.ts Outdated Show resolved Hide resolved
external/js/cothority/src/byzcoin/v2/darcsBS.ts Outdated Show resolved Hide resolved
@ineiti
Copy link
Member Author

ineiti commented Jul 16, 2020

  • v1 / v2 -> for the moment I'd like to keep v1, so for making it clearer, change byzcoin/v2 to v2/byzcoin, and add eventual other services to it, like v2/skipchain

@ineiti
Copy link
Member Author

ineiti commented Jul 16, 2020

I also prefer not to use the following:

export namespace Coin {
    export class BehaviorSubject {}
    export class Instance {}
    export namespace Contract {}
}

Because of the collision of the names Coin, BehaviorSubject, and Instance. So I build things with the following structure:

export class CoinBS{}
export class CoinInst{}
export namespace CoinContract{}

Again, CoinInstance is already taken in v1, and to avoid unnecessary suffering of the users, I don't want to use the same name.

Another set I see is:

  • Always the latest data with: CoinInst extends BehaviorSubject<CoinVersion>
  • One version of the data: CoinVersion extends Coin
  • Contract interactions: CoinContract

But I don't like the CoinVersion too much.

@tharvik
Copy link
Contributor

tharvik commented Jul 16, 2020

Again, CoinInstance is already taken in v1, and to avoid unnecessary suffering of the users, I don't want to use the same name.

So you expect that people mixes v1 & v2? Then, is it really a new version or more of an extension (so in something like byzcoin-bs)?

Another set I see is:

  • Always the latest data with: CoinInst extends BehaviorSubject<CoinVersion>

That's weird that a stream of data is an instance; CoinStream maybe or CoinUpdates?
I like to play with Struct & Structs, the first being an element, the second being a stream/list/container of the first.

  • One version of the data: CoinVersion extends Coin

CoinUpdate?

@ineiti
Copy link
Member Author

ineiti commented Jul 17, 2020

So you expect that people mixes v1 & v2? Then, is it really a new version or more of an extension (so in something like byzcoin-bs)?

Well it all is in the @dedis/cothority package, so if two classes have the same name, but come from a different path, confusion is guaranteed.

That's weird that a stream of data is an instance; CoinStream maybe or CoinUpdates?

That's the ByzCoin terminology: an instance points to data, and the contract defines how this data can be changed.

I like to play with Struct & Structs, the first being an element, the second being a stream/list/container of the first.

OK, mixing it all well:

  • CoinStruct - containing Coin and readonly inst byzcoin.Instance
  • CoinStream / CoinInst - BehaviorSubject<CoinStruct>
  • CoinContract - constants and command-generators

So the CoinInst would be in reference to the byzcoin-instance, which by definition is changeable.

@tharvik
Copy link
Contributor

tharvik commented Jul 17, 2020

So you expect that people mixes v1 & v2? Then, is it really a new version or more of an extension (so in something like byzcoin-bs)?

Well it all is in the @dedis/cothority package, so if two classes have the same name, but come from a different path, confusion is guaranteed.

Right, that makes it confusing for some people..

  • CoinStruct - containing Coin and readonly inst byzcoin.Instance
  • CoinStream / CoinInst - BehaviorSubject<CoinStruct>
  • CoinContract - constants and command-generators

So the CoinInst would be in reference to the byzcoin-instance, which by definition is changeable.

That looks more consistent, more readable; good for you?

@ineiti ineiti force-pushed the add_txcollector branch 2 times, most recently from 086d3a1 to 1855108 Compare July 20, 2020 14:12
@ineiti
Copy link
Member Author

ineiti commented Jul 20, 2020

yes, starts to look OK for me. However, now it complains about the following:

export namespace CoinContract {

with

ERROR: src/v2/byzcoin/contracts/coinInst.ts:108:1 - 'namespace' and 'module' are disallowed

Perhaps I'll make it a class with only static fields and methods anyway?

@tharvik
Copy link
Contributor

tharvik commented Jul 20, 2020

Hum, it seems that when using module, you can't use namespace, you should put any namespace into a new module (ie a new file) and export that you want, which is semantically the same

@ineiti
Copy link
Member Author

ineiti commented Jul 21, 2020

Seems to be one of those eternal PRs...

@ineiti ineiti force-pushed the add_txcollector branch 6 times, most recently from 84cabb3 to b0ad039 Compare July 28, 2020 06:32
@ineiti ineiti marked this pull request as ready for review July 28, 2020 06:33
@ineiti ineiti force-pushed the add_txcollector branch 2 times, most recently from acb217d to 2322d14 Compare July 28, 2020 07:11
@ineiti ineiti force-pushed the add_txcollector branch 3 times, most recently from f175be6 to 520ce65 Compare October 16, 2020 17:52
@ineiti ineiti marked this pull request as ready for review October 16, 2020 19:25
@ineiti ineiti moved this from WIP to Ready4Merge in Cothority Oct 16, 2020
@ineiti ineiti moved this from Ready4Merge to WIP in Cothority Oct 19, 2020
@ineiti ineiti changed the base branch from master to improve_getUpdates October 19, 2020 11:43
@ineiti ineiti force-pushed the add_txcollector branch 2 times, most recently from a2de364 to d23a295 Compare October 19, 2020 13:34
Base automatically changed from improve_getUpdates to master October 19, 2020 15:03
@ineiti ineiti moved this from WIP to Ready4Merge in Cothority Oct 19, 2020
The previous requests and instances were difficult to use for multi-instruction transactions.
Also, they didn't use the new Observable interface given by ByzCoin.

This PR adds a v2/ directory with some soon-to-be stable version of Darc and Coin instances.
Every instance can now be created as a BehaviorSubject that updates automatically whenever
a new block is available.
This allows to program a much more reactive programming of the user-interface: instead of
polling for new values, the UI can subscribe to the BehaviorSubjects and be updated
whenever something changes.

In the long run, other contracts should also find their way into byzcoin/v2.
if (BCTest.bct === undefined) {
BCTest.bct = await BCTest.init();
} else {
await new Promise((resolve) => setTimeout(resolve, 1000));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why sleep for a second?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, as the test are stopping at the first failed assert, dropping this line would show a test not cleaning correctly or even missing some relevant check. Maybe it was added during developpement and isn't needed now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, ran it 78 times until travis-timeout without the sleep...

@ineiti ineiti marked this pull request as draft October 22, 2020 10:56
@ineiti ineiti moved this from Ready4Merge to WIP in Cothority Oct 22, 2020
@ineiti ineiti moved this from WIP to Ready4Merge in Cothority Oct 22, 2020
@ineiti ineiti marked this pull request as ready for review October 22, 2020 13:17
@ineiti ineiti requested review from tharvik and removed request for cgrigis October 22, 2020 13:17
@tharvik tharvik merged commit a3dc9b4 into master Oct 22, 2020
Cothority automation moved this from Ready4Merge to Closed Oct 22, 2020
@tharvik tharvik deleted the add_txcollector branch October 22, 2020 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Cothority
  
Closed
Development

Successfully merging this pull request may close these issues.

None yet

2 participants