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

1484 net v2 #1586

Draft
wants to merge 116 commits into
base: main
Choose a base branch
from
Draft

1484 net v2 #1586

wants to merge 116 commits into from

Conversation

lucanicoladebiasi
Copy link
Collaborator

@lucanicoladebiasi lucanicoladebiasi commented Dec 17, 2024

Description

This code provides in packages/net a new paradigm to interact with Thor.
The modules aims to substitute the functionalities in packages/net.

The code is purely experimental and will be discussed in the SDK workshop of 14th-15th January in Dublin.

This is a very active development, receiving multiple pushes per day.

** DO NOT MERGE ANYWHERE**

The prototype follows a strict principle I believe an open source SDK connecting with web-services should adhere: all what published as web-service documentation should have its class or method in the SDK. As SDK user persona I want to see the REST or WS documentation and I want to see those elements are represented in the SDK. Then, the SDK can offer me a lot more things, but I should be free to build additional things on the base of elements allowing me to have in the code using the SDK the elements published by Thor documentation.

The prototype has some known defects and limitations. The most important are the following ones.

  • There is a lot of confusion among what block, blockref, thor and tx id should be.
  • An undetected problem in the exporting from core module obliged to import few classes through relative paths, that is rubbish. Together, we can fix such problem.
  • The code is not documented.
  • The tests are just examples, those are not complete.
  • The author of the prototype is a well known Object Oriented bigot: sometime he adds classes in an hierarchy when not strictly needed for sake of ontologic UML clarity and extendibility. How much severe is his disease, let's discuss in the workshop.

/**
* This class is an alias of {@link TxId} for back compatibility.
*/
class TxId extends BlockId {
Copy link
Member

Choose a reason for hiding this comment

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

theoretically why does a TxId extend a BlockId
to me should be independant - even though they have the same hex number of digits etc

Also the doc comment on the class links to itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm confused between the concepts of ThorId and TxId and BlockId. From the regex point of view those are the same thing: 64 hexadecimal digits. Are they conceptually different? Or should just have a ThorId class to express block and tx identifier? Let's discuss together.

*
* @param {TxId} blockId - The unique identifier for the block.
*/
protected constructor(blockId: BlockId) {
Copy link
Member

Choose a reason for hiding this comment

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

variable name doesnt match doc comment , why would i create a TxId from a BlockId?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't pay attention to doc and have many second and third thoughts about block, thor and tx ids.
See above comment.

}
}

export { BlockId, ThorId, TxId };
Copy link
Member

Choose a reason for hiding this comment

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

what is thorId ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See above, let's discuss.

@@ -0,0 +1,18 @@
import { InvalidDataType } from '@vechain/sdk-errors';

class Int extends Number {
Copy link
Member

Choose a reason for hiding this comment

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

Needs a doc comment

}

public static isValid0x(exp: string): boolean {
return HexUInt.REGEX_HEXUINT_PREFIX.test(exp) && Nonce.isValid(exp);
Copy link
Member

Choose a reason for hiding this comment

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

whats the purpose of this? as HexUInt.REGEX_HEXUINT_PREFIX.test(exp) is applied twice

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

worth to verify if it is a bug

import { type ThorRequest } from '../ThorRequest';
import { type ThorResponse } from '../ThorResponse';

import { type TxId } from '../../../../core/src/vcdm/BlockId';
Copy link
Member

Choose a reason for hiding this comment

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

to improve import

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some classes were not exported correctly from core, I don't understand why. This is just a workaround.

}

get query(): string {
return this.head === undefined ? '' : `${this.head}&`;
Copy link
Member

Choose a reason for hiding this comment

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

perhaps HttpQuery class can generate this

@@ -0,0 +1,21 @@
import { ThorId } from '@vechain/sdk-core';

class TXID {
Copy link
Member

Choose a reason for hiding this comment

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

dont we have TxId type elsewhere? in VCDM package

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This class is different and mimic https://mainnet.vechain.org/doc/stoplight-ui/#/schemas/TXID, we shold change one of the two names. See my comment about Tx, Thor, Block id.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, but the two classes are different and this name reflects https://mainnet.vechain.org/doc/stoplight-ui/#/. One of the two names should change and we should think wisely if TxId is just ThotID or BlockId.

import { type WebSocketListener } from './WebSocketListener';
import { type HttpPath } from '../http';

class MozillaWebSocketClient implements WebSocketClient {
Copy link
Member

Choose a reason for hiding this comment

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

Why name Mozilla? When should this be used

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Becase I used te standard Mozilla JS API and I suffered lack of fantasy in a better name. Let's decide a better name.

@@ -0,0 +1,3 @@
export * from './MozillaWebSocketClient';
Copy link
Member

Choose a reason for hiding this comment

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

perhaps named exports for bundle optimisation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

blah, yes, workaround...

import { Hex } from './Hex';
import { InvalidDataType } from '@vechain/sdk-errors';

class Nonce extends HexUInt {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs a doc comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I postponed any doc writing.

code: string;
}

class ContractBytecode {
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can add docs here too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No doc yet, had no time.

httpClient: HttpClient
): Promise<ThorResponse<RetrieveConnectedPeers, GetPeersResponse>> {
const response = await httpClient.get(RetrieveConnectedPeers.PATH, {
query: ''
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure if we need query here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

workaround because prototype, we can do it better

@@ -0,0 +1,47 @@
import { BlockId } from '@vechain/sdk-core';

import { UInt } from '../../../../core/src/vcdm/UInt';
Copy link
Collaborator

Choose a reason for hiding this comment

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

relative imports

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

...are horrible, but something in the imported core package prevents me to import correctly. half of the comment are about this

@@ -0,0 +1,31 @@
import { BlockId } from '@vechain/sdk-core';

import { UInt } from '../../../../core/src/vcdm/UInt';
Copy link
Collaborator

Choose a reason for hiding this comment

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

import

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

... it's rubbish!

Copy link
Collaborator Author

@lucanicoladebiasi lucanicoladebiasi left a comment

Choose a reason for hiding this comment

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

The prototype follows a strict principle I believe an open source SDK connecting with web-services should adhere: all what published as web-service documentation should have its class or method in the SDK. As SDK user I want to see the REST or WS documentation and I want to see those elements are represented in the SDK. Then, the SDK can offer me a lot more things, but i can be free to build additional things on the base of elements allowing me to have in the code using the SDK the elements published by Thor documentation.

The prototype has some known defects and limitations. The most important are

  • There is a lot of confusion among what block, blockref, thor and tx id should be.
  • An undetected problem in the exporting from core module obliged me to import few classes through relative paths, that is rubbish, but i hadn't any quicker solution.
  • The code is not documented.
  • The tests are just examples, those are not complete.
  • I'm an Object Oriented bigot: sometime I add classes in an hierarchy when not strictly needed for sake of ontology UML clarity and extendibility. I do believe abstraction is a virtue, not a defect because I was educated in a grammar school and I was poisoned by logic and philosophy courses.

request: this,
response: new ExecuteCodesResponse(responseBody)
};
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

x

/**
* This class is an alias of {@link TxId} for back compatibility.
*/
class TxId extends BlockId {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm confused between the concepts of ThorId and TxId and BlockId. From the regex point of view those are the same thing: 64 hexadecimal digits. Are they conceptually different? Or should just have a ThorId class to express block and tx identifier? Let's discuss together.

*
* @param {TxId} blockId - The unique identifier for the block.
*/
protected constructor(blockId: BlockId) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't pay attention to doc and have many second and third thoughts about block, thor and tx ids.
See above comment.

}
}

export { BlockId, ThorId, TxId };
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See above, let's discuss.

import { type ThorRequest } from '../ThorRequest';
import { type ThorResponse } from '../ThorResponse';

import { type TxId } from '../../../../core/src/vcdm/BlockId';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some classes were not exported correctly from core, I don't understand why. This is just a workaround.

import { ReceiptOutput, type ReceiptOutputJSON } from './ReceiptOutput';
import { Address, Hex, HexUInt, Units, VTHO } from '@vechain/sdk-core';

class Receipt {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The prototype follows a close parallelism with what published at https://mainnet.vechain.org/doc/stoplight-ui/#/.

I strongly do believe the SDK must present the same data model the REST documentation publishes, at least at some level. As SDK user I want to see an SDK I can match with the REST end-point at first.
Then, It's absolute appropriate the SDK provides additional level of composition and abstraction using the lower level objects the REST points document.


import { TxId } from '../../../../core/src/vcdm/BlockId';

class ReceiptMeta extends TxMeta {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As Object oriented bigot, it extends and extension is not a matter of convenience. Let's see the UML gospel about.

import { type ThorRequest } from '../ThorRequest';
import { type ThorResponse } from '../ThorResponse';

import { type TxId } from '../../../../core/src/vcdm/BlockId';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

... when yarn allows me to import correctly or you tell me what I did wrong exporting types.

import { type ThorRequest } from '../ThorRequest';
import { type ThorResponse } from '../ThorResponse';

import { type TxId } from '../../../../core/src/vcdm/BlockId';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see above...

@@ -0,0 +1,21 @@
import { ThorId } from '@vechain/sdk-core';

class TXID {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, but the two classes are different and this name reflects https://mainnet.vechain.org/doc/stoplight-ui/#/. One of the two names should change and we should think wisely if TxId is just ThotID or BlockId.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants