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

ExternalLedgerDB.Wallet schema #806

Closed
wants to merge 17 commits into from

Conversation

Projects
None yet
3 participants
@unnawut
Copy link
Collaborator

commented Feb 14, 2019

Issue/Task Number: #690
Closes #690
Requires #807

Overview

This PR adds a new ExternalLedgerDB app to manage schemas and database rules for all external ledgers that we may support. Currently namely Ethereum network and OMG plasma network.

Changes

  • Upgraded :ecto_sql to fix Ecto ignoring :queue_target and :queue_interval options
  • Added a new ExternalLedgerDB sub-app
  • Added ExternalLedgerDB.Wallet and its related migration and tests
  • Added Utils.Ledgers.WalletSchema behaviour and enforce on ExternalLedgerDB.Wallet and LocalLedgerDB.Wallet

Implementation Details

This is a straight-forward addition of a new sub-app and ExternalLedgerDB.Wallet schema.

A Utils.Ledgers.WalletSchema was added and enforced on ExternalLedgerDB.Wallet and LocalLedgerDB.Wallet with anticipation that the EWallet and its related apps will be interacting with local & external wallets interchangeably. With this behavioural enforcement we should be able to do the routing between the two ledgers much easier than without the enforcement.

Usage

Not yet. DB schema and migrations only. No API to interact with.

Impact

New DB migration. No API specs changed.

@unnawut unnawut added this to the v1.2 milestone Feb 14, 2019

@unnawut unnawut requested review from sirn, jarindr, T-Dnzt, ripzery and mederic-p Feb 14, 2019

@ghost ghost added the s2/wip 🚧 label Feb 14, 2019

@unnawut unnawut added s3/review 👀 and removed s2/wip 🚧 labels Feb 15, 2019

@omg_network "omg_network"

def ethereum, do: @ethereum
def omg_network, do: @omg_network

This comment has been minimized.

Copy link
@T-Dnzt

T-Dnzt Feb 15, 2019

Member

This shouldn't be hardcoded. The adapters should have a way to register themselves.

:metadata,
:encrypted_metadata
])
|> validate_inclusion(:adapter, [@ethereum, @omg_network])

This comment has been minimized.

Copy link
@T-Dnzt

T-Dnzt Feb 15, 2019

Member

Ethereum and the OMG network are not exclusive, they work together. The same wallet (same Ethereum address) can have funds on Ethereum AND Plasma at the same time. This should validate the inclusion within the registered list of adapters instead. We'll later have an adapter that will basically be ethereum + omg_network (but we could also have an adapter just for ethereum, which will be the case at first).

This comment has been minimized.

Copy link
@unnawut

unnawut Feb 18, 2019

Author Collaborator

A bit confused. I was thinking that each ExternalLedgerDB.Wallet should belong to one adapter…

And the same wallet with same address on ethereum and omg_plasma would be considered two ExternalLedgerDB.Wallet, because technically they share nothing apart from the same pub/priv keys? (which may not be true too for other chains). The system doesn’t need to know that they are related?

Also… do you mean a wallet.adapter should be an array like wallet.adapter == ["ethereum", "omg_network"]? If so I feel there’ll be a lot of conditioning based on that array compared to 1 wallet 1 adapter.

This comment has been minimized.

Copy link
@T-Dnzt

T-Dnzt Feb 21, 2019

Member

As we discussed, I see Plasma as more of an extension of the Ethereum adapter than its own chain. It can't work without it, and funds for an Eth wallet will either be on the main chain or the sidechain (well, still in the main chain really, just in the plasma smart contract).

|> cast(attrs, [
:address,
:adapter,
:primary,

This comment has been minimized.

Copy link
@T-Dnzt

T-Dnzt Feb 15, 2019

Member

Should we have a specific changeset for the primary hot wallet? Since it won't happen often. An alternative would be to store it in the settings instead of inside the DB. Thoughts?

This comment has been minimized.

Copy link
@unnawut

unnawut Feb 18, 2019

Author Collaborator

Hmm I like the idea of having it as a settings instead. 👍

This comment has been minimized.

Copy link
@T-Dnzt

T-Dnzt Feb 21, 2019

Member

Alright, so it'll be like the master account, let's do it 👍

Show resolved Hide resolved apps/utils/lib/ledgers/wallet_schema.ex Outdated

@unnawut unnawut force-pushed the 690-blockchain-wallet-schema branch from 22d5f87 to ada52d3 Feb 26, 2019

@unnawut

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 1, 2019

Closing this PR as an ExternalLedgerDB is probably not needed for now.

@unnawut unnawut closed this Mar 1, 2019

@ghost ghost removed the s3/review 👀 label Mar 1, 2019

@unnawut

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 1, 2019

Will submit a new PR for blockchain wallet handling.

@unnawut unnawut deleted the 690-blockchain-wallet-schema branch Mar 1, 2019

@unnawut unnawut referenced this pull request Mar 27, 2019

Closed

Blockchain wallet schema #690

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.