Conversation
- current implementation was mistakenly only looking at the first ABI - this approach also panics if there's a mismatch since tests often squash the message that would explain the exit on fatal log - also compare parsed ABI so we don't fail due to different formatting
- Not a valid expectation when dealing with the same event across several types of contract (e.g. flap/flip/flop)
| if a.Constructor.String() != b.Constructor.String() { | ||
| return false | ||
| } | ||
| OuterMethods: |
There was a problem hiding this comment.
I cannot think of a better way to write this code. But I sure wish I could.
| ) | ||
|
|
||
| var _ = Describe("External constants", func() { | ||
| var flipABI = `[{"inputs":[{"internalType":"address","name":"vat_","type":"address"},{"internalType":"bytes32","name":"ilk_","type":"bytes32"}],"payable":false,"stateMutability":"nonpayable","type":"constructor"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"uint256","name":"id","type":"uint256"},{"indexed":false,"internalType":"uint256","name":"lot","type":"uint256"},{"indexed":false,"internalType":"uint256","name":"bid","type":"uint256"},{"indexed":false,"internalType":"uint256","name":"tab","type":"uint256"},{"indexed":true,"internalType":"address","name":"usr","type":"address"},{"indexed":true,"internalType":"address","name":"gal","type":"address"}],"name":"Kick","type":"event"},{"anonymous":true,"inputs":[{"indexed":true,"internalType":"bytes4","name":"sig","type":"bytes4"},{"indexed":true,"internalType":"address","name":"usr","type":"address"},{"indexed":true,"internalType":"bytes32","name":"arg1","type":"bytes32"},{"indexed":true,"internalType":"bytes32","name":"arg2","type":"bytes32"},{"indexed":false,"internalType":"bytes","name":"data","type":"bytes"}],"name":"LogNote","type":"event"},{"constant":true,"inputs":[],"name":"beg","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"internalType":"uint256","name":"","type":"uint256"}],"name":"bids","outputs":[{"internalType":"uint256","name":"bid","type":"uint256"},{"internalType":"uint256","name":"lot","type":"uint256"},{"internalType":"address","name":"guy","type":"address"},{"internalType":"uint48","name":"tic","type":"uint48"},{"internalType":"uint48","name":"end","type":"uint48"},{"internalType":"address","name":"usr","type":"address"},{"internalType":"address","name":"gal","type":"address"},{"internalType":"uint256","name":"tab","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[{"internalType":"uint256","name":"id","type":"uint256"}],"name":"deal","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":false,"inputs":[{"internalType":"uint256","name":"id","type":"uint256"},{"internalType":"uint256","name":"lot","type":"uint256"},{"internalType":"uint256","name":"bid","type":"uint256"}],"name":"dent","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":false,"inputs":[{"internalType":"address","name":"usr","type":"address"}],"name":"deny","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":false,"inputs":[{"internalType":"bytes32","name":"what","type":"bytes32"},{"internalType":"uint256","name":"data","type":"uint256"}],"name":"file","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[],"name":"ilk","outputs":[{"internalType":"bytes32","name":"","type":"bytes32"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[{"internalType":"address","name":"usr","type":"address"},{"internalType":"address","name":"gal","type":"address"},{"internalType":"uint256","name":"tab","type":"uint256"},{"internalType":"uint256","name":"lot","type":"uint256"},{"internalType":"uint256","name":"bid","type":"uint256"}],"name":"kick","outputs":[{"internalType":"uint256","name":"id","type":"uint256"}],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[],"name":"kicks","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[{"internalType":"address","name":"usr","type":"address"}],"name":"rely","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[],"name":"tau","outputs":[{"internalType":"uint48","name":"","type":"uint48"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[{"internalType":"uint256","name":"id","type":"uint256"},{"internalType":"uint256","name":"lot","type":"uint256"},{"internalType":"uint256","name":"bid","type":"uint256"}],"name":"tend","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":false,"inputs":[{"internalType":"uint256","name":"id","type":"uint256"}],"name":"tick","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[],"name":"ttl","outputs":[{"internalType":"uint48","name":"","type":"uint48"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[],"name":"vat","outputs":[{"internalType":"contract VatLike","name":"","type":"address"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"internalType":"address","name":"","type":"address"}],"name":"wards","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[{"internalType":"uint256","name":"id","type":"uint256"}],"name":"yank","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"}]` |
There was a problem hiding this comment.
Do these tests need a full IRL ABI? It's a lot of noise.
There was a problem hiding this comment.
I think the answer is, unfortunately, yes - since the actual functions are reading from the testing.toml config. In theory we could put a shorter fake ABI in there to simplify but, despite the file's name, we have historically had it exactly match the other config in terms of transformers/contracts. Part of the motivation there is that we also use it for integration tests.
| func GetContractsABI(contractNames []string) string { | ||
| func GetABIFromContractsWithMatchingABI(contractNames []string) string { | ||
| initConfig() | ||
| if len(contractNames) < 1 { |
There was a problem hiding this comment.
Does initConfig take time? You could assert before the initConfig to save that.
There was a problem hiding this comment.
I'm not sure whether it ever takes any significant amount of time, but it seems possible that it could - given that it's performing file reads. Could you elaborate on how you'd structure an assert? Wondering if we already achieve the same goal with the initialized var inside initConfig() (which flips to true the first time it's called and halts subsequent runs).
There was a problem hiding this comment.
What I mean is - couldn't you check len(contractNames) before calling init?
paytonrules
left a comment
There was a problem hiding this comment.
I don't see any blockers. The code is weird because it's hard to understand the point of it - but it looks correct.
elizabethengelman
left a comment
There was a problem hiding this comment.
Looks good. Just a couple questions.
| } | ||
| return false | ||
| } | ||
| OuterEvents: |
There was a problem hiding this comment.
What is this called - when you have OuterMethods and OuterEvents defined like this within a method? I feel like I've seen this before, but can't remember how it works.
There was a problem hiding this comment.
I've seen it referred to here as a label break
There was a problem hiding this comment.
It's a GOTO and golang is too chicken to admit it.
| } | ||
|
|
||
| // GetFirstABI returns the ABI from the first contract in a collection in config | ||
| func GetFirstABI(contractNames []string) string { |
There was a problem hiding this comment.
Where is this method being used?
There was a problem hiding this comment.
That's now used when generating the transformer config - since we have events that live across contracts with different ABIs (e.g. deal on flap/flip/flop)
iterating through:
for _, contractName := range contractNames[:1]means we only ever look at the first passed ABI. Fixing that led to failures where formatting differed, so we're now parsing the ABI before comparison. This required a custom function to do the comparison since equality comparisons aren't implemented onabi.ABI. Decided to panic if that fails so that we can see the error message (didn't happen with the oldlog.Fatalapproach if the test suite was redirecting the logs toioutil.Discard).