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

multi: create on-disk storage of addresses, and new RPC calls to: create, query, and decode them #73

Merged
merged 10 commits into from
Aug 3, 2022

Conversation

Roasbeef
Copy link
Member

In this PR, we create the initial scaffolding to address creation, querying and decoding. This is a line of PRs towards fully e2e send+recv functionality within the daemon.

On disk we store a simple unrolled address table. In the future, we can use these rows, similar to the "seedling" table (for asset creation) to create an asset stub in the DB. With the stub created, we'd then start to look for the taproot output that anchors the said asset.

We add three calls to the CLI tool:

  • taro addr new
  • taro addr query
  • taro addr decode

Here's all three calls in action:

⛰   ./tarocli-debug --network=simnet addrs new --asset_id=e625b840faf0628e2f20625a12c2 --amt=100
{
    "addr": "tarosb1qqqsqq3qucjmss867p3guteqvfdp9ssqqqqqqqqqqqqqqqqqqqqqqqqqqqqqggzcpqdxfhqctal3a939jfzqrg8pwt7t905kxz3x3adzx0jezq2uc5rzql083qh8ww9juzaeuxzvcxjyy8qlmwzfwskvywezrzyt4spyyr3upqqkg632gl0"
}

⛰   ./tarocli-debug --network=simnet addrs q
{
    "addrs": [
        {
            "addr": "tarosb1qqqsqq3qucjmss867p3guteqvfdp9ssqqqqqqqqqqqqqqqqqqqqqqqqqqqqqggr2da3l9xhsk8qempd5ayd6txx8g4hlzagp8j8a8jm7zj8tzw0mrqrzpx2yx9t63l7ckgvw56s4zngktndmczehppka7kk4humufj29m9jnpqqkgf66nfj"
        },
        {
            "addr": "tarosb1qqqsqq3qucjmss867p3guteqvfdp9ssqqqqqqqqqqqqqqqqqqqqqqqqqqqqqggzcpqdxfhqctal3a939jfzqrg8pwt7t905kxz3x3adzx0jezq2uc5rzql083qh8ww9juzaeuxzvcxjyy8qlmwzfwskvywezrzyt4spyyr3upqqkg632gl0"
        }
    ]
}

⛰   ./tarocli-debug --network=simnet addrs decode tarosb1qqqsqq3qucjmss867p3guteqvfdp9ssqqqqqqqqqqqqqqqqqqqqqqqqqqqqqggr2da3l9xhsk8qempd5ayd6txx8g4hlzagp8j8a8jm7zj8tzw0mrqrzpx2yx9t63l7ckgvw56s4zngktndmczehppka7kk4humufj29m9jnpqqkgf66nfj
{
    "asset_genesis": {
        "version": 0,
        "genesis_point": "",
        "name": "",
        "meta": null,
        "asset_id": "e625b840faf0628e2f20625a12c2000000000000000000000000000000000000"
    },
    "asset_type": "NORMAL",
    "amount": "100",
    "lock_time": 0,
    "relative_lock_time": 0,
    "script_version": 0,
    "script_key": "6a6f63f29af0b1c19d85b4e91ba598c7456ff175013c8fd3cb7e148eb139fb18",
    "asset_family": {
        "raw_family_key": null,
        "tweaked_family_key": null,
        "asset_id_sig": null
    },
    "chain_anchor": null
}

Fixes #63

Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM, straightfwd changes, only a few nits and questions from me 🚀

// ParamsForChain returns the ChainParams for a given chain based on its name.
func ParamsForChain(name string) ChainParams {
switch name {
case chaincfg.MainNetParams.Name:
Copy link
Member

Choose a reason for hiding this comment

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

q: i've seen this elsewhere int this code base that style is slightly different from the lnd style guide. Like compact switch/cases, no extra space for multi line function signatures etc. Is this intentional or should reviewers point it out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, in this case intentional since this is sort of a mechanical mapping, that could even really just be a static map. Typically when the case statements are doing something non-trivial, I'd usually add more spacing to add comments, etc there.

no extra space for multi line function signatures etc.

In this case I usually add a space here, but ultimately we might need to add some additional entries to the style guide since it's been a while since it's been updated.

tarodb/sqlite/migrations/000003_addrs.up.sql Outdated Show resolved Hide resolved
// the TaroAddressBook book. We need to be able to insert/fetch addresses, and
// also make internal keys since each address has an internal key and a script
// key (tho they can be the same).
type AddrBook interface {
Copy link
Member

Choose a reason for hiding this comment

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

nit: naming wise, perhaps better to be more explicit and also not use abbreviation here? AddrBook => TaroAddressStore. If you agree similar comments for below types.

Copy link
Member Author

Choose a reason for hiding this comment

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

So ultimately, I think we should rename the package to taroaddr (cc @jharveyb). This way the contents aren't ambiguous with the import path taroaddr.Leaf (or w/e). Then this would be taroaddr.Book which is less ambiguous and taroaddr.Store.

tarodb/addrs.go Outdated Show resolved Hide resolved
tarodb/addrs.go Show resolved Hide resolved
cmd/tarod/main.go Show resolved Hide resolved
key_ring.go Outdated Show resolved Hide resolved
key_ring.go Outdated Show resolved Hide resolved
*/
int64 created_after = 1;

/*
Copy link
Member

Choose a reason for hiding this comment

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

nit: I never know what style is preferred so looked it up in lightning.proto and we usually don't mix different comment types. Maybe we could just use // for all comments?

Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC we need this type for multi-line columns over 80 chars? Though something might have changed, this is how it was in the past, maybe @guggero knows?

cmd/tarocli/addrs.go Outdated Show resolved Hide resolved
@dstadulis
Copy link
Collaborator

FYI: decode output is missing the asset_key

@dstadulis
Copy link
Collaborator

#64 could be incorporated to this

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

tarodb/sqlite/migrations/000003_addrs.down.sql Outdated Show resolved Hide resolved
// Similarly, for sqlite using LIMIT with a value of -1 means no rows
// should be limited.
//
// TODO(roasbeef): needs to be more portable
Copy link
Member

Choose a reason for hiding this comment

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

I'll let this work in my subconscious for a bit to find a solution for more portable code. Somehow search/replace (as was the idea for the BYTEA vs. BLOB problem) doesn't seem like a scalable solution. Maybe we can introduce the concept of language specific macros/definitions to sqlc?
For example:

postgres.definitions

BYTEARRAY = BYTEA
LIMITNULL = LIMIT NULLIF($1, -1)

sqlite.definitions

BYTEARRAY = BLOB
LIMITNULL = LIMIT $1

queries.sql

SELECT example FROM wherever
LIMITNULL($1);

Generated queries.postgres.sql:

SELECT example FROM wherever
LIMIT NULLIF($1, -1);

We would then generate code for both SQLite and Postgres and the Querier interface would more or less handle the abstraction of the two backends.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to double check, this "definitions" construct doesn't exist in SQL today right?

Re limit:

  • For postgres you can pass NULL
  • While for sqlite you need to pass -1 to mean now limit

However we could also just do like math.MaxUint32 or something like that, which would work for both DB backends.

Somehow search/replace (as was the idea for the BYTEA vs. BLOB problem) doesn't seem like a scalable solution.

Long term I think so, AFAICT, this is the only thing we need to search/replace, given we use the postgres code gen backend, and all our queries so far are also valid postgres queries. So I think we can minimize the search/replace to just the table definitions (🤞) and try to write portable SQL beyond that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or do you mean we'd take that definitions struct and use that to search-and-replace within the queries?

Copy link
Member

Choose a reason for hiding this comment

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

I meant we could add a PR to the sqlc library to add this concept of language specific definitions (like pre-processor macros in C). But maybe that's overkill and we can solve it in different ways. Was just an idea.

tarorpc/taro.proto Outdated Show resolved Hide resolved
cmd/tarocli/addrs.go Outdated Show resolved Hide resolved
@Roasbeef Roasbeef merged commit 8a865e3 into main Aug 3, 2022
@guggero guggero deleted the addr-book branch August 3, 2022 07:47
dstadulis pushed a commit that referenced this pull request May 16, 2023
proof: move proof receiver ack detection into backoff procedure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

rpc: add a DecodeAddress call
5 participants