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

Add types spec #2

Closed
wants to merge 8 commits into from
Closed

Conversation

chabroA
Copy link
Contributor

@chabroA chabroA commented Sep 6, 2022

Add specification for types based on https://github.com/LedgerHQ/live-app-sdk/blob/main/docs/reference/modules.md

It looks like some types are not related to the specification (are more implementation specific), like EcdsaSignature, ExchangeDeviceTxId or ExchangePayload.

Also, some types are actually not used anywhere today (like ApplicationDetails or DeviceInfo).

Some types are also Ledger Live specific like RawSignedTransaction.

It feels like all these types should be either removed from the spec or reworked (like RawSignedTransaction that seems quite useless as a type since the object it represents is juste passed through the Client SDK from signTransaction to broadcastSignedTransaction.

Same general feeling regarding types, some types are JS specific (like BigNumber or Buffer). We should represent them in a language agnostic way (how? what type should we use?)

What do you think? What's your opinion? How should we go about that?

Justkant and others added 6 commits August 30, 2022 09:49
also cleanup some folders
Add a paragraph introducting the Client API to give context and link to
the RPC spec that the Client needs to implement
Provide information regarding the need to serialize and deserialize some
types in the client before and after interaction with the server
Provide information about error handling by the Client
@chabroA chabroA requested review from Justkant and a team September 6, 2022 13:39
@chabroA chabroA force-pushed the support/LIVE-3593-create-types-spec branch from ec8e9f4 to 2d4eabd Compare September 6, 2022 13:40
Copy link
Collaborator

@Justkant Justkant left a comment

Choose a reason for hiding this comment

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

Nice j'ai pas fait le tour de chaque type mais ça à l'air clean comme ça, on devra aussi update la spec rpc avec des liens vers le bon type

@chabroA chabroA requested a review from a team September 7, 2022 08:28
@Justkant Justkant self-requested a review September 7, 2022 08:28
Copy link
Contributor

@sprohaszka-ledger sprohaszka-ledger left a comment

Choose a reason for hiding this comment

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

I wonder if we should not explain what is a raw type and its purpose, unless it's common sense for everyone.

@chabroA
Copy link
Contributor Author

chabroA commented Sep 9, 2022

I wonder if we should not explain what is a raw type and its purpose, unless it's common sense for everyone.

Good idea 👍

@chabroA
Copy link
Contributor Author

chabroA commented Sep 9, 2022

Added note regarding raw types ed4ba34

For now I limited it to JSON primitive types (Strings, Numbers, Booleans, and Null) but there might not be any technical limitation to use structured types (Objects and Arrays).

With that said, if we can use Objects, then raw types might not be useful since everything can be represented by an Object 🤔

@chabroA
Copy link
Contributor Author

chabroA commented Sep 13, 2022

recreated here #7

Wozacosta added a commit that referenced this pull request Jan 19, 2024
signandbroadcast handles tokencurrency

test account receive

remove local manifest test
Wozacosta added a commit that referenced this pull request Jan 19, 2024
…289)

* handle empty token accounts in transaction.sign #2

signandbroadcast handles tokencurrency

test account receive

remove local manifest test

* remove change manifest wapitools

* changeset

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

Successfully merging this pull request may close these issues.

3 participants