-
Notifications
You must be signed in to change notification settings - Fork 2
ERN spec #149
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
ERN spec #149
Conversation
proto/ddex/v1beta2/common.proto
Outdated
// address of the party sending the message, must match the recovered address from the signature | ||
string from = 2; | ||
// address of the party receiving the message, must be empty for new release messages | ||
string to = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make as a message type
pkg/core/server/ern.go
Outdated
} | ||
|
||
func (s *Server) finalizeERNNewMessage(ctx context.Context, req *abcitypes.FinalizeBlockRequest, txhash string, messageIndex int64, ern *v1beta2.ElectronicReleaseNotification) error { | ||
nonce := fmt.Sprintf("%d", ern.Header.Nonce) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt.Sprint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing blocking for me. it is a lot to grok, but with the right entrypoint, it's pretty easy to follow. tests helped a lot
pkg/common/address.go
Outdated
"google.golang.org/protobuf/proto" | ||
) | ||
|
||
func keccak256(data []byte) []byte { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this helper, you can use go-ethereum
import (
"github.com/ethereum/go-ethereum/crypto"
)
and just call
hash := crypto.Keccak256(message)
@@ -3,6 +3,7 @@ package pages | |||
import ( | |||
"encoding/json" | |||
v1 "github.com/AudiusProject/audiusd/pkg/api/core/v1" | |||
v1beta1 "github.com/AudiusProject/audiusd/pkg/api/core/v1beta1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kind of a nit, but i don't love the v1beta
nomenclature. it's just going to to be hard to change in the future. why not call it something more generic , like ddex
or wire_protocol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my vote is ddex, so just remove the subpackage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a standard way to communicate versioning in protobuf
https://google.aip.dev/185
https://buf.build/docs/best-practices/style-guide/#package-structure
BlockHeight int64 | ||
} | ||
|
||
type CorePie struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love me a slice of that core pie
@@ -169,6 +185,31 @@ type CoreEtlTxValidatorRegistration struct { | |||
CreatedAt pgtype.Timestamptz | |||
} | |||
|
|||
type CoreMead struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drink drank drunk
create index if not exists idx_core_ern_message_control_type on core_ern (message_control_type); | ||
create index if not exists idx_core_ern_sender on core_ern (sender); | ||
|
||
create index if not exists idx_core_ern_party_addresses_gin on core_ern using gin (party_addresses); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why these gin indexes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they were added in a previous version to support querying the text[] columns, i can remove them until we decide on a path forward there
return ErrV2TransactionExpired | ||
} | ||
|
||
// TODO: check signature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this todo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EIP 712 isn't implemented nor agreed upon yet, this is where that logic check would go
Signature: sig, | ||
DataHash: sigData, | ||
Data: data, | ||
// create ERN track release with upload cid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure I understand why we deleted the old tests, those should still work right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh i see now, it's b/c you removed the impl of that method for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, and signed stream urls will change this entirely. this test doesn't test any relevant functionality for us yet
}, | ||
}) | ||
ernReceipt := submitRes.Msg.TransactionReceipt.MessageReceipts[0].GetErnAck() | ||
require.NotNil(t, ernReceipt, "failed to get ern ack") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we assert more here? like the address that gets generated? that should be deterministic right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kind of, we don't know which block this ERN will be included in so we can guess but not predict.
Nonce: "4", | ||
Expiration: time.Now().Add(time.Hour).Unix(), | ||
}, | ||
Messages: []*corev1beta1.Message{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a great test
}, | ||
}, | ||
{ | ||
PartyReference: "P_ARTIST_5189", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛈️ ⛈️ ⛈️ ⛈️ ⛈️ ⛈️
Uh oh!
There was an error while loading. Please reload this page.