Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

delete by prefix rfc #39

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

yoni-wolf
Copy link
Contributor

Signed-off-by: Yoni yoni.wolf@intel.com

Signed-off-by: Yoni <yoni.wolf@intel.com>
[summary]: #summary

This RFC proposes an additional API to transaction context,
context.deletePrefix(address_prefix) which will get an address prefix as input

Choose a reason for hiding this comment

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

Can we extend existing API and allow partial address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could.
I thought in order to prevent TP developers from deleting prefix by mistake, the right approach will be to add a new API for this instead of adding capabilities to existing delete API.
Do you think it will be better to allow delete with prefix by extending existing API?

Choose a reason for hiding this comment

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

Yes, this can however be controlled by outputs field in TransactionHeader.

Copy link
Contributor

Choose a reason for hiding this comment

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

What would extending the existing API look like?

Copy link
Contributor

Choose a reason for hiding this comment

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

@yoni-wolf please correct me, but I think that would look like the existing state delete call would take a list of address prefixes rather than a list of addresses. The validator would then need to distinguish each element of the list as between a full 70 character address as a single address or something less than 70 as a prefix.
Combining this: https://github.com/hyperledger/sawtooth-core/blob/master/protos/state_context.proto#L66-L70
With this: https://github.com/hyperledger/sawtooth-core/blob/master/protos/state_context.proto#L66-L70
@vaporos if that's not what you were asking please elaborate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Clarification was that this option should be included in the alternatives section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to the alternative section the option to use existing API instead of adding a new API

@agunde406 agunde406 self-assigned this Mar 12, 2019
text/0000-delete-prefix.md Outdated Show resolved Hide resolved
Signed-off-by: Yoni <yoni.wolf@intel.com>
text/0000-delete-prefix.md Outdated Show resolved Hide resolved
# Prior art
[prior-art]: #prior-art

Access address by prefix is a key capability of radix tree.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is worded badly. In fact, I don't know that I would go so far as to even include prior art, since it is an extension of current Sawtooth - the Merkle-Radix trie - which that functionality made use of prior art.

@agunde406 agunde406 self-requested a review March 13, 2019 14:13
Signed-off-by: Yoni <yoni.wolf@intel.com>
Signed-off-by: Yoni <yoni.wolf@intel.com>
@agunde406 agunde406 self-requested a review April 11, 2019 12:57
@vaporos
Copy link
Contributor

vaporos commented Apr 18, 2019

In several places it looks like two paragraphs are touching and it's not clear if they are meant to be separate paragraphs or whether it's one paragraph with sloppy line wrapping.

In several places Sawtooth should be capitalized.

@dcmiddle dcmiddle requested review from agunde406 and removed request for ineffectualproperty, aludvik and TomBarnes April 18, 2019 15:05
Signed-off-by: ywolf <yoni.wolf@intel.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants