-
Notifications
You must be signed in to change notification settings - Fork 132
RPC: Add Ignore Tree Endpoints #1554
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
RPC: Add Ignore Tree Endpoints #1554
Conversation
Pull Request Test Coverage Report for Build 16472678428Details
💛 - Coveralls |
asset/asset.go
Outdated
@@ -569,6 +606,10 @@ func (id PrevID) Hash() [sha256.Size]byte { | |||
return *(*[sha256.Size]byte)(h.Sum(nil)) | |||
} | |||
|
|||
// OutPoint is a type alias for an asset outpoint. It links the anchor outpoint | |||
// to the asset it secures. | |||
type OutPoint = PrevID |
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.
PrevOut
?
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.
Or AnchorPoint
(to be used as asset.AnchorPoint
which reads nicely IMO) to not confuse it with a purely on-chain wire.OutPoint
?
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.
Looks pretty good, did a quick first pass.
asset/asset.go
Outdated
@@ -569,6 +606,10 @@ func (id PrevID) Hash() [sha256.Size]byte { | |||
return *(*[sha256.Size]byte)(h.Sum(nil)) | |||
} | |||
|
|||
// OutPoint is a type alias for an asset outpoint. It links the anchor outpoint | |||
// to the asset it secures. | |||
type OutPoint = PrevID |
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.
Or AnchorPoint
(to be used as asset.AnchorPoint
which reads nicely IMO) to not confuse it with a purely on-chain wire.OutPoint
?
661466c
to
32da29f
Compare
I've rebased on to branch |
32da29f
to
6fbc382
Compare
d7efd50
to
c2c394f
Compare
37bdcfa
to
b5a6c5e
Compare
b5a6c5e
to
c59b408
Compare
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.
Nice work! Comments mostly related to extra validation or error reporting we can do at the RPC call site.
|
||
// Fetch the full delegation key locator. | ||
delegationKey, err := r.cfg.TapAddrBook.FetchInternalKeyLocator( | ||
ctx, &delegationPubKey, |
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.
So this'll reliably fail if we weren't the ones that actually issued the asset right? Or will signing fail below?
Either way, we should amke thsi check and the related failure explicit.
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.
I've made the endpoint's error message more explicit and extended the integration test to include a case where a non-owner node attempts to create a signed ignore tuple and correctly fails.
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.
Did you forget to push this up maybe? I don't see the addition to the itests.
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.
} | ||
} | ||
|
||
require.True(t.t, foundCommitTxOut) |
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.
Yeah the syncer/verifier isn't hooked up yet.
543c669
to
8654ab9
Compare
Adds a method to generate a Schnorr signature over the TLV serialization of an IgnoreTuple.
This functionality will be used in the next commit which adds the IgnoreAssetOutPoint RPC endpoint.
ff1e6db
to
2994e96
Compare
2994e96
to
786df50
Compare
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.
Very nice, LGTM 🎉
Introduce a new RPC endpoint to add an asset outpoint to the asset's ignore tree. This allows asset issuers to explicitly exclude certain asset outpoints.
Introduce the UpdateSupplyCommit endpoint to trigger the creation and broadcast of a new on-chain supply commitment transaction for a given asset group. This allows clients to manually update supply commitments as needed.
Modified FetchSupplyCommit to include metadata from the mined Bitcoin block containing the supply commit transaction.
Updated the RootCommitment struct to include metadata from the mined Bitcoin block.
Add FetchSupplyCommit endpoint to retrieve the on-chain supply commitment for a specific asset group. The response includes optional inclusion proofs for any provided leaf keys.
Add CLI commands to: - Ignore an asset outpoint in the supply commitment, - Trigger a manual on-chain update of the supply commitment, - Fetch the current supply commitment state.
Replaced usages of the LeafNode concrete type with the more general Node interface in function signatures where applicable. This adds flexibility which will become useful in a future itest.
Update the sendAssetAndAssert integration test helper to return the RPC response from the SendAsset call. This allows callers to access the full response data for further assertions or processing.
Add integration test to ensure universe supply commitments properly handle ignored asset outpoints. The test confirms that once an outpoint is marked as ignored, it is correctly placed in the ignore subtree of the supply commitment, and that this state is reflected in the mined commitment transaction.
786df50
to
1fdd9da
Compare
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 🐈⬛
Closes #1534
EDIT: depends on #1655
Sharing early to get early feedback on naming and architecture. Still missing itests and query RPC endpoint.
This PR will not include the verifier, I will add that in a separate PR.
This PR adds RPC endpoints to populate and query from ignore trees.