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

Add new keys and getter / setter functions #7378

Conversation

chatton
Copy link
Contributor

@chatton chatton commented Oct 2, 2024

Description

NOTE: explicitly not closing linked issue, as these fields are subject to change pending final spec.

With these we can continue with implementations of new handler functions.

ref: #7366

closes #7132


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against the correct branch (see CONTRIBUTING.md).
  • Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
  • Code follows the module structure standards and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/).
  • Added relevant godoc comments.
  • Provide a conventional commit message to follow the repository standards.
  • Include a descriptive changelog entry when appropriate. This may be left to the discretion of the PR reviewers. (e.g. chores should be omitted from changelog)
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Review SonarCloud Report in the comment section below once CI passes.

@@ -15,6 +15,7 @@ import (

"github.com/cosmos/ibc-go/v9/modules/core/04-channel/v2/types"
host "github.com/cosmos/ibc-go/v9/modules/core/24-host"
hostv2 "github.com/cosmos/ibc-go/v9/modules/core/24-host/v2"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new keys have been added under a host/v2

// PacketReceiptKey returns the store key of under which a packet
// receipt is stored
func PacketReceiptKey(sourceID string, sequence uint64) []byte {
return []byte(fmt.Sprintf("receipts/channels/%s/sequences/%d", sourceID, sequence))
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 can introduce utility functions if needed, but I didn't think it was necessary here

Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

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

ack though q about sequences which I recall in PR were to be stored using big endian

}

// GetNextSequenceSend returns the next send sequence from the sequence path
func (k *Keeper) GetNextSequenceSend(ctx context.Context, sourceID string) (uint64, bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

unblocks #7327

@@ -61,3 +62,96 @@ func (k *Keeper) GetCounterparty(ctx context.Context, clientID string) (types.Co
k.cdc.MustUnmarshal(bz, &counterparty)
return counterparty, true
}

// GetPacketReceipt returns the packet receipt from the packet receipt path based on the sourceID and sequence.
func (k *Keeper) GetPacketReceipt(ctx context.Context, sourceID string, sequence uint64) (string, bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

oh, weren't all sequences in keys now to be stored using big endian?

Copy link
Contributor

Choose a reason for hiding this comment

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

fine if issue for later too (might already exist and I missed it)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sequences should be in big endian as specified by @DimitrisJim. Is there a way to specify that in the go code? Which is the default option in go?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@sangier sangier left a comment

Choose a reason for hiding this comment

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

Apart from the comment on the sequences, the implementation seems to properly reflect the specs! Thank you @chatton!!

Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Nice, looks super clean! I thought it would be good to raise the question around sequence formatting here and see what folks think

Copy link
Member

Choose a reason for hiding this comment

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

With new keys we should probably consider how we format the sequence in order to support effecient iteration, right?
I think this was an issue before, as formatting ended up in lexographical ordering rather than numerical(?)

E.g. format sequence using big endian bytes rather than decimal string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes! Was just chatting to @DimitrisJim about it, seems like we do indeed want to use BigEndian, pushing changes now.

Copy link
Member

Choose a reason for hiding this comment

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

Should've refreshed my browser before publishing review... 😆

// PacketReceiptKey returns the store key of under which a packet
// receipt is stored
func PacketReceiptKey(sourceID string, bigEndianSequence []byte) []byte {
return []byte(fmt.Sprintf("receipts/channels/%s/sequences/%s", sourceID, string(bigEndianSequence)))
Copy link
Contributor

Choose a reason for hiding this comment

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

%s implicitly calls string on arg no?

Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

ACK! Thanks @chatton!

// GetPacketCommitment returns the packet commitment hash under the commitment path.
func (k *Keeper) GetPacketCommitment(ctx context.Context, sourceID string, sequence uint64) (string, bool) {
store := k.storeService.OpenKVStore(ctx)
bigEndianBz := sdk.Uint64ToBigEndian(sequence)
Copy link
Member

Choose a reason for hiding this comment

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

don't want to be nitting too hard but might be nicer to do this inside host pkg. API looks friendlier with a uint64 sequence, and the formatting is a host responsibility imo 🤷🏻

Copy link

sonarcloud bot commented Oct 2, 2024

Quality Gate Failed Quality Gate failed for 'ibc-go'

Failed conditions
65.3% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@chatton chatton merged commit 17d3487 into feat/ibc-eureka Oct 2, 2024
65 of 66 checks passed
@chatton chatton deleted the cian/issue#7366-add-store-functions-to-the-channelkeeper-v2-that-use-the-new-provable-paths branch October 2, 2024 13:17
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.

4 participants