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

delete by prefix rfc#39

Open
yoni-wolf wants to merge 5 commits intohyperledger-archives:mainfrom
yoni-wolf:delete-prefix
Open

delete by prefix rfc#39
yoni-wolf wants to merge 5 commits intohyperledger-archives:mainfrom
yoni-wolf:delete-prefix

Conversation

@yoni-wolf
Copy link
Copy Markdown
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
Copy link
Copy Markdown

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
Copy Markdown
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?

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Signed-off-by: Yoni <yoni.wolf@intel.com>
# Prior art
[prior-art]: #prior-art

Access address by prefix is a key capability of radix tree.
Copy link
Copy Markdown
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>
@agunde406 agunde406 requested a review from peterschwarz April 3, 2019 12:46
Signed-off-by: Yoni <yoni.wolf@intel.com>
@agunde406 agunde406 self-requested a review April 11, 2019 12:57
@vaporos
Copy link
Copy Markdown
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 TomBarnes, aludvik and ineffectualproperty 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants