Skip to content

(VDB-1039) Add pot join event transformer#68

Merged
gslaughl merged 1 commit intostagingfrom
vdb-1039-pot-join
Dec 16, 2019
Merged

(VDB-1039) Add pot join event transformer#68
gslaughl merged 1 commit intostagingfrom
vdb-1039-pot-join

Conversation

@gslaughl
Copy link
Copy Markdown
Contributor

No description provided.

Comment on lines +33 to +36
common.HexToHash("0x049878f300000000000000000000000000000000000000000000000000000000"),
common.HexToHash("0x000000000000000000000000e7bc397dbd069fc7d0109c0636d06888bb50668c"),
common.HexToHash("0x000000000000000000000000000000000000000000000000417fa3222791bd1a"),
common.HexToHash("0x0000000000000000000000000000000000000000000000000000000000000000"),
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.

These topics are from a real kovan event (the one used in integration tests), and I just thought it was weird how it has 3 topics even though we're only expecting 2. I'm probably missing some subtlety of solidity, but it seemed worth mentioning

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.

I think this is because LogNote always includes 4 topics, regardless of how many topics are actually used by an individual note modifier. Hopefully we could (theoretically) omit this field and everything would still behave correctly.

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.

Yeah everything does seem to work correctly if we omit the 3rd and 4th topics, but the part that seemed weird to me was that the 3rd topic isn't empty. In other events shaped the same way (e.g. vat heal events), the 3rd and 4th topics are all zeroes, like the 4th one is here.

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.

Oh wait I think topics[2] is the wad value we want. Pretty sure topics[1] is the msg.sender: https://github.com/makerdao/dss/blob/master/src/lib.sol#L37

Copy link
Copy Markdown
Contributor

@rmulhol rmulhol left a comment

Choose a reason for hiding this comment

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

:shipit:

Comment on lines +33 to +36
common.HexToHash("0x049878f300000000000000000000000000000000000000000000000000000000"),
common.HexToHash("0x000000000000000000000000e7bc397dbd069fc7d0109c0636d06888bb50668c"),
common.HexToHash("0x000000000000000000000000000000000000000000000000417fa3222791bd1a"),
common.HexToHash("0x0000000000000000000000000000000000000000000000000000000000000000"),
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.

I think this is because LogNote always includes 4 topics, regardless of how many topics are actually used by an individual note modifier. Hopefully we could (theoretically) omit this field and everything would still behave correctly.

var rawPotJoinLog = types.Log{
Address: common.HexToAddress(PotAddress()),
Topics: []common.Hash{
common.HexToHash("0x049878f300000000000000000000000000000000000000000000000000000000"),
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.

Totally nbd but I think we conventionally populate topic0 with the signature from the constants. Can't think of any reason that matters other than consistency, though

func TestPotJoin(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "PotJoin Suite")
}
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.

I was going to say we should logrus.SetOutput(ioutil.Discard), but that actually doesn't seem necessary. I wonder if we can delete this from other places if transformers are just converters and converters aren't logging? 🤔

@gslaughl gslaughl force-pushed the vdb-1039-pot-join branch 2 times, most recently from e7f90af to 279ad2a Compare December 16, 2019 19:29
@gslaughl gslaughl merged commit f2ce4bc into staging Dec 16, 2019
@gslaughl gslaughl deleted the vdb-1039-pot-join branch December 16, 2019 20:04
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.

2 participants