-
Notifications
You must be signed in to change notification settings - Fork 133
[group key addrs 7/7]: send and receive support for V2 addresses #1587
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
Conversation
a5c2fd0
to
b482ea3
Compare
b482ea3
to
8f8e164
Compare
094b31e
to
3d1a126
Compare
8f8e164
to
060159b
Compare
dc426c6
to
74953ff
Compare
db7f36c
to
a3dbdd3
Compare
5687720
to
46d9159
Compare
a3dbdd3
to
ad2c782
Compare
e5fdf7e
to
eea552e
Compare
ae1aefb
to
cd9b477
Compare
eea552e
to
555c711
Compare
As a preparation for creating events from wire transactions that aren't notified by the lnd wallet (and therefore don't come through the lndclient calls), we make the arguments to GetOrCreateEvents a bit more explicit.
We refactor the proof handling to allow multiple outputs to be processed and proofs for them to be fetched.
Simplifies the code a bit by using the Goroutine helper method.
c691660
to
9b0821a
Compare
Okay, the itest failures are constant now, not flakes. Must be something I changed in the last push. Will address first thing tomorrow. |
9b0821a
to
a45379a
Compare
Found the itest issue, should be good to merge now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🦜
Reviewed 43 of 63 files at r10, 20 of 20 files at r11, all commit messages.
Reviewable status: all files reviewed, 60 unresolved discussions (waiting on @ffranr, @guggero, and @ZZiigguurraatt)
itest/addrs_v2_test.go
line 50 at r11 (raw file):
amountToRequest := uint64(0) amountToSend := a.Amount - 1 if amountToSend == 0 {
What type of error would get user get if they attempted to send a value greater than 1 to a collectible address? I wonder if we should encode that extra bit into the addr type.
FWIW, calls to decode the address should fetch the genesis as needed allowing this full information to be displayed.
tapdb/sqlc/queries/authmailbox.sql
line 52 at r6 (raw file):
m.encrypted_payload, m.arrival_timestamp, m.expiry_block_height,
Ah so we're removing this everywhere after all in favor the the should-be-good-enough-for-everyone value? np, we can always re add it later in a migration if needed
rpcserver.go
line 1689 at r11 (raw file):
// If the user didn't specify a specific address version but is using // a group key, we default to version 2.
👍
proof/courier.go
line 1878 at r11 (raw file):
// RPC methods that we require. statusErr, ok := status.FromError(err) if ok && statusErr.Code() == codes.Unimplemented {
Nice, I assume this is for cases where a Universe server hasn't updated to the latest version?
cmd/commands/addrs.go
line 122 at r11 (raw file):
// at least a V2 address. So we can just use that if the user // specifies no explicit version. if len(groupKey) > 0 {
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 60 unresolved discussions (waiting on @ffranr, @Roasbeef, and @ZZiigguurraatt)
itest/addrs_v2_test.go
line 50 at r11 (raw file):
Previously, Roasbeef (Olaoluwa Osuntokun) wrote…
What type of error would get user get if they attempted to send a value greater than 1 to a collectible address? I wonder if we should encode that extra bit into the addr type.
FWIW, calls to decode the address should fetch the genesis as needed allowing this full information to be displayed.
Good question!
So due to this:
taproot-assets/address/address.go
Line 219 in 07d0c50
case asset.Collectible: |
We don't allow addresses for collectibles to have a zero (optional) amount in the first place. So the error would already be thrown at address creation time:
unable to make new addr: unable to make new addr: address: collectible asset amount not one
.
I've added an integration test case for this!
proof/courier.go
line 1878 at r11 (raw file):
Previously, Roasbeef (Olaoluwa Osuntokun) wrote…
Nice, I assume this is for cases where a Universe server hasn't updated to the latest version?
Yes, correct.
tapdb/sqlc/queries/authmailbox.sql
line 52 at r6 (raw file):
Previously, Roasbeef (Olaoluwa Osuntokun) wrote…
Ah so we're removing this everywhere after all in favor the the should-be-good-enough-for-everyone value? np, we can always re add it later in a migration if needed
Yes, exactly. We can always add something later once we know exactly how/when we can do a cleanup without affecting emergency recovery.
Adds the new --addrs_with_amounts flag that allows to specify the amount to send to an address that allows sending user-defined amounts (address V2).
For v2 addresses we want the address to only show the group key if a group key is used. The value of the asset ID becomes meaningless for an address that supports group keys. Unfortunately we can't really _remove_ the value from the address (it's always encoded). But we can set it to all zeroes so it's more apparent to the user that it's not used.
a45379a
to
a7b40ad
Compare
The re-try mechanism introduced in a previous PR didn't work properly, because the re-try only happens if the callback returns an error. This fixes the behavior by returning a temporary error to trigger the re-try.
This PR introduces a new V2 address that:
This change is