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

Remove signature metadata action #358

Merged
merged 17 commits into from
Aug 30, 2022
Merged

Remove signature metadata action #358

merged 17 commits into from
Aug 30, 2022

Conversation

HagarMeir
Copy link
Contributor

Signed-off-by: Hagar Meir [email protected]

@HagarMeir HagarMeir linked an issue Aug 17, 2022 that may be closed by this pull request
8 tasks
@HagarMeir HagarMeir marked this pull request as ready for review August 22, 2022 07:20
@HagarMeir HagarMeir removed a link to an issue Aug 22, 2022
8 tasks
@@ -189,6 +189,6 @@ func (t *TransferAction) Deserialize(raw []byte) error {
}

// GetMetadata returns the claim pre-image
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please change this comment

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@@ -207,8 +210,8 @@ func (t *TransferAction) IsGraphHiding() bool {

// GetMetadata returns metadata of the TransferAction
// in zkatdlog it returns the claim pre-image
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please change comment

Copy link
Contributor

Choose a reason for hiding this comment

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

done

IsGraphHiding() bool
GetMetadata() []byte
// GetMetadata returns the action's metadata
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add a dot at the end of the comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we usually don't have it. No?

if logger.IsEnabledFor(zapcore.DebugLevel) {
logger.Debugf("expected key without the claim pre-image, skipping")
logger.Debugf("expected key without the issue action metadata, skipping")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

issue-->transfer

Copy link
Contributor

Choose a reason for hiding this comment

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

okay

@@ -41,6 +41,7 @@ const (
SerialNumber = "sn"
IssueActionMetadata = "iam"
TransferActionMetadata = "tam"
ClaimPreImage = "cpi"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you can remove this?

Copy link
Contributor

Choose a reason for hiding this comment

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

okay

@adecaro adecaro removed their request for review August 25, 2022 07:09
token/core/fabtoken/actions.go Outdated Show resolved Hide resolved
token/core/fabtoken/validator_transfer.go Outdated Show resolved Hide resolved
}
// is it owned by a htlc script?
if owner.Type == htlc.ScriptType {
// Then, the first output must be compatible with this input.
Copy link
Contributor

Choose a reason for hiding this comment

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

the transfer action should include one output.

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure about this. What should I change?

return errors.Errorf("invalid transfer action: an htlc script only transfers the ownership of a token")
}

// check type and quantity
Copy link
Contributor

Choose a reason for hiding this comment

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

for an ownership transfer

Copy link
Contributor

Choose a reason for hiding this comment

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

can you specify the full error message, please?

token/core/fabtoken/validator_transfer.go Outdated Show resolved Hide resolved
token/services/vault/keys/keys.go Show resolved Hide resolved
token/services/vault/translator/action.go Outdated Show resolved Hide resolved
Copy link
Contributor

@KElkhiyaoui KElkhiyaoui left a comment

Choose a reason for hiding this comment

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

looks good to me.

}

out := ctx.Action.GetOutputs()[0].(*token.Token)
if ctx.InputTokens[0].Data.Equals(out.Data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This check should be removed as it is incorrect. ZKVerifier takes care of checking if the transfer of ownership is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where is this check done exactly?

@HagarMeir HagarMeir merged commit a6b159e into main Aug 30, 2022
@HagarMeir HagarMeir deleted the sig-md branch September 7, 2022 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants