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

Algod: rename some API operations for clarity #4376

Merged
merged 5 commits into from
Aug 9, 2022

Conversation

Aharonee
Copy link
Contributor

@Aharonee Aharonee commented Aug 9, 2022

No description provided.

@Aharonee Aharonee requested review from id-ms and almog-t August 9, 2022 12:46
@Aharonee Aharonee self-assigned this Aug 9, 2022
@codecov
Copy link

codecov bot commented Aug 9, 2022

Codecov Report

Merging #4376 (2a41360) into master (2bc55c0) will increase coverage by 0.03%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #4376      +/-   ##
==========================================
+ Coverage   55.56%   55.60%   +0.03%     
==========================================
  Files         403      403              
  Lines       50776    50780       +4     
==========================================
+ Hits        28216    28237      +21     
+ Misses      20163    20151      -12     
+ Partials     2397     2392       -5     
Impacted Files Coverage Δ
daemon/algod/api/server/v2/handlers.go 0.00% <0.00%> (ø)
libgoal/libgoal.go 2.72% <ø> (ø)
catchup/service.go 68.88% <0.00%> (-0.50%) ⬇️
util/db/dbutil.go 50.30% <0.00%> (+1.21%) ⬆️
crypto/merkletrie/node.go 93.48% <0.00%> (+1.86%) ⬆️
agreement/proposalManager.go 98.03% <0.00%> (+1.96%) ⬆️
agreement/cryptoVerifier.go 69.71% <0.00%> (+2.11%) ⬆️
crypto/merkletrie/trie.go 68.61% <0.00%> (+2.18%) ⬆️
ledger/tracker.go 76.49% <0.00%> (+3.84%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@Aharonee Aharonee changed the title StateProofs: rename GetProof to GetTransactionProof to avoid ambiguity Algod: rename some API operations for clarity Aug 9, 2022
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas3.yml Outdated Show resolved Hide resolved
Comment on lines 1257 to +1264
StateProof: protocol.Encode(&tx.StateProof),
}

response.Message.BlockHeadersCommitment = tx.Message.BlockHeadersCommitment
response.Message.VotersCommitment = tx.Message.VotersCommitment
response.Message.LnProvenWeight = tx.Message.LnProvenWeight
response.Message.FirstAttestedRound = tx.Message.FirstAttestedRound
response.Message.LastAttestedRound = tx.Message.LastAttestedRound
Copy link
Contributor

@winder winder Aug 9, 2022

Choose a reason for hiding this comment

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

nit: initialize all at once

Suggested change
StateProof: protocol.Encode(&tx.StateProof),
}
response.Message.BlockHeadersCommitment = tx.Message.BlockHeadersCommitment
response.Message.VotersCommitment = tx.Message.VotersCommitment
response.Message.LnProvenWeight = tx.Message.LnProvenWeight
response.Message.FirstAttestedRound = tx.Message.FirstAttestedRound
response.Message.LastAttestedRound = tx.Message.LastAttestedRound
StateProof: protocol.Encode(&tx.StateProof),
Message: generated.StateProof.Message {
BlockHeadersCommitment: tx.Message.BlockHeadersCommitment,
VotersCommitment: tx.Message.VotersCommitment,
LnProvenWeight: tx.Message.LnProvenWeight,
FirstAttestedRound: tx.Message.FirstAttestedRound,
LastAttestedRound: tx.Message.LastAttestedRound,
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

the message is defined as an embedded struct so this code will not work as is.
the idea was to avoid exposing a public structure named message since it is not very informative by itself. Rather have it embedded into the state proof response

Copy link
Contributor

Choose a reason for hiding this comment

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

You can still do these sorts of things with embedded structs: https://go.dev/play/p/prtHNzb7xPl

Copy link
Contributor

Choose a reason for hiding this comment

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

But this - does not https://go.dev/play/p/FquXrvHyzeM
Message is generated as embedded:

type StateProof struct {

	// Represents the message that the state proofs are attesting to.
	Message struct {

		// The vector commitment root on all light block headers within a state proof interval.
		BlockHeadersCommitment []byte `json:"BlockHeadersCommitment"`

"format": "byte",
"pattern": "^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$",
"type": "string"
"description": "Represents the message that the state proofs are attesting to.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a little more than a rename, were these fields just missing before?

Copy link
Contributor

@id-ms id-ms Aug 9, 2022

Choose a reason for hiding this comment

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

the state proof message could be retrieved only in its msgpack version. I think it would be great to have all fields parsed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants