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

EIP-0012 - dApp-Wallet Web Bridge #23

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rooooooooob
Copy link

Technical spec for wallet-dApp communication.

Technical spec for wallet-dApp communication.
@rooooooooob rooooooooob marked this pull request as draft August 10, 2020 12:47
eip-0012.md Outdated Show resolved Hide resolved
eip-0012.md Outdated Show resolved Hide resolved
eip-0012.md Outdated Show resolved Hide resolved
eip-0012.md Outdated

### Value

BigNum-like object. Represents a value which may or may not be ERG. Value is in the smallest possible unit (e.g. nanoErg for ERG). It can be either a `number` or a `string` using standard unsigned integer text representation.
Copy link
Member

Choose a reason for hiding this comment

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

Value is a positive but encoded as i64 number actually.

Copy link
Member

Choose a reason for hiding this comment

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

@kushti Actually it was a "raw" u64 initially, but now I made a newtype BoxValue(u64)

Copy link
Author

Choose a reason for hiding this comment

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

If it's positive somewhere (even if Long/i64 encoded), then it should be fine as-is here, right? I didn't look into how Scala handles Long deserialization from JSON for numbers too big to fit, but in sigma-rust the wrapped u64 should take in a string-encoded uint like "1053546432154" afaik. Please correct me if I'm wrong and this should be changed.

Copy link
Member

Choose a reason for hiding this comment

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

Scala representation of Long is signed, so max value is 2^63-1.

Copy link
Author

Choose a reason for hiding this comment

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

For JSON deserialization in the full node is this description fine though? Wouldn't the max possible value for nanoergs be much smaller than this? I guess for other token types it could be relevant - in which case should we keep it as-is but specify that it cant' be more than a 64-bit signed number or what?

eip-0012.md Outdated Show resolved Hide resolved
@gagarin55
Copy link
Contributor

I suggest you to specify data types in language independent way, not types from sigma-rust implementation

@rooooooooob
Copy link
Author

I suggest you to specify data types in language independent way, not types from sigma-rust implementation

We do specify the types in language-independant JSON. This was intended to mirror what sigma-rust had at the time for ease of interopability, but they are specified at the bottom of the document.

…ementation of it in Yoroi.

Type definitions are specified using flow types now instead of some
pseudo JS schema.

API errors are added to all API calls to handle things that can go wrong
for any call,e.g. internal error in the wallet, incorrect params
specified to API calls.

get_change_address() was added as an explicit API since it should be
easy to support and allows for an easier way during tx creation rather
than arbitrarily choosing an address from the used or unused API calls,
which aren't garuanteed to be non-empty.

add_external_box was removed as we are still unsure how any wallets
would be able to implement it in any meaningful way. This could be
reversed and have most wallets treat it as a no-op.
@rooooooooob
Copy link
Author

I made some minor changes based on issues encountered during our initial implementation inside of Yoroi:

Type definitions are specified using flow types now instead of some
pseudo JS schema.

API errors are added to all API calls to handle things that can go wrong
for any call,e.g. internal error in the wallet, incorrect params
specified to API calls.

get_change_address() was added as an explicit API since it should be
easy to support and allows for an easier way during tx creation rather
than arbitrarily choosing an address from the used or unused API calls,
which aren't guaranteed to be non-empty.

add_external_box() was removed as we are still unsure how any wallets
would be able to implement it in any meaningful way. This could be
reversed and have most wallets treat it as a no-op.

We removed BoxCandidate in favor of just Box to make things simpler. It also allows interop with sigma-rust/full nodes for now.

It would be ideal to not reference sigma-rust in any way for things that are more specific than just the underlying javascript type for example:

Uses `sigma-rust` rep - Hex-encoded bytes for `SigmaSerialize` of a constant.

which specify things like encoding, length, etc.

The id field has been removed, as well as the tx id/box id from outputs
which are now BoxCandidates.

This was only used before to help with interop with exisitng tooling but
now that sigma-rust no longer requires these there's no reason to keep
them around. They only made it more difficult to construct an unsigned
transaction. Now one can be a lot more easily constructed even without
using other tooling to calculate the IDs/etc.
@rooooooooob
Copy link
Author

BoxCandidate was restored as it was only removed to ease support with sigma-rust/for simplicity, but the additional information was never necessary. Now that the data model was updated in sigma-rust it makes more sense to revert that change to the original BoxCandidate model and remove tx IDs from the unsigned Tx which makes it potentially easier to construct by the dApp.

Full box contents are now included in UnsignedTransaction to help light wallets support P2S inputs + for all wallets in processing recently created outputs. Wallets can check that the content matches the ID so there should be no security issues with that.

eip-0012.md Outdated Show resolved Hide resolved
typo in type name

Co-authored-by: Denys Zadorozhnyi <[email protected]>

### Box

Transaction output box. Format is the same as `sigma-rust`:
Copy link
Member

Choose a reason for hiding this comment

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

There is no Box in sigma-rust. I suppose ErgoBox is meant here.


A candidate (no TX/box IDs) for an output in an `UnsignedTransaction`
```
type BoxCandidate = {|
Copy link
Member

@greenhat greenhat Jul 22, 2021

Choose a reason for hiding this comment

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

Probably should mention that it's ErgoBoxCandidate in sigma-rust?


### SignedTx

Uses `sigma-rust` rep for a transaction:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Uses `sigma-rust` rep for a transaction:
Uses `sigma-rust` `Transaction` rep for a transaction:

@rooooooooob
Copy link
Author

We're currently also developing a similar spec to this for Cardano here. There has been a lot of discussion there, some of which is relevant to Ergo. Here are some things that we might want to cover here too:

  • Handling multiple wallets installed (for Cardano we solve this by injecting into a namespaced object - for Ethereum they just tell you to only have one installed e.g. MetaMask)
  • Versioning (i.e. if this spec gets updated and some API changes)
  • Phishing prevention (this could be assumed to be a wallet implementation detail, e.g. showing some user-known secret image in connector popups to stop phishing sites from creating similar looking popups to a user's wallet to farm passwords or other info)


The proposed API is limited to just the minimum wallet <-> dApp communication needed rather than providing lots of utility functions (tx building, data conversions, etc) or node-access (for UTXO scanning). This is different compared to web3 as that functionality could be modular in a separate library and Ergo smart contracts don't need as much direct node access for basic dApp functionality compared to Ethereum.

This API is accessible from a javascript object `ergo` that is injected into the web context upon wallet consent. Before this just the free `ergo_request_read_access()` and `ergo_check_read_access()` functions are injected into the web context. An event handler can also be registered to the web context to detect wallet disconnects tagged as `"ergo_wallet_disconnected"`.

Choose a reason for hiding this comment

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

In order to avoid conflicts with multiple wallets, can we do a slight change on the access API? Maybe something like ergo.{walletName}.requestReadAccess() and ergo.{walletName}.checkReadAccess()

Copy link
Author

Choose a reason for hiding this comment

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

Yeah that's what we ended up doing for CIP-0030 for Cardano which I based off of this. I'm not sure if it's fine to do such a breaking change at this point since this has been up for a long time although it's still a draft PR. I'll wait and see what the Ergo devs think about this. We also don't have any functionality for versioning either. I mentioned both of these in my comment above in August. There are additional events that can be of use for dApps as well that could be added.

Copy link
Member

Choose a reason for hiding this comment

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

Agree with @capt-nemo429 . Explicitly distinguishing wallets in the dApp connector API is a crucial feature. Shouldn't be too problematic to migrate.

eip-0012.md Show resolved Hide resolved
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.

None yet

8 participants